r52 / PTA

PoE Trade Assistant
GNU General Public License v3.0
21 stars 3 forks source link

Feature Request: Show trade results and price in the search window itself #25

Closed Rottenbeer closed 4 years ago

Rottenbeer commented 4 years ago

I found https://github.com/redSol/PoeTradeSearchEN, which is the english translation of a korean Trade Searcher and has some nice features, like the search being included in the trade window itself:

Price Listings

Do you think it's possible to add this to PTA?

r52 commented 4 years ago

In terms of possibility, of course it's possible 😄 . In fact in terms of feasibility, you probably wouldn't even need to change the backend ItemAPI code too much in order to support something like this, because JSON is used to pass around data most of the time even internally, so adding backend support for a feature like this would be relatively easy.

However, the frontend (UI and such) is a completely different story. (Re)implementing all of the UI elements in C++ to support these features is quite a big undertaking if I were to do this by myself, and I have no motivation right now to do a big rewrite/overhaul of all the UI because I've already stopped playing this league and I hate writing UI code. Those darned C# apps have the benefit of having streamlined UI tooling rolled into their language 🤣 .

Implementation wise, I'd consider one of these 3 options, along with what I think the pros and cons would be for each option:

1. Enhance current StatDialog

One option is to simply copy this Korean app's UI and implement all the corresponding elements into the current StatDialog class as C++.

Pros (not really)

2. Re-implement StatDialog as QML

The next option is to rewrite StatDialog (and possibly the price template as well, if we intend to merge simple search into the same dialog to keep uniform UI elements) using Qt's QML markup language that includes these features from the ground up.

Pros

3. Re-implement StatDialog and price template as a web app

The third option is to rewrite StatDialog as well as the price template as a web app. Preferably as pure HTML/CSS/JS in order to take advantage of libraries already available to PTA (Qt WebEngine), but adding a node.js/Electron backend to PTA wouldn't be difficult (although I wouldn't recommend this because adding an engine for one piece of UI seems not worth and would probably lead to a lot of extra bloat).

Pros


Personally, I'd choose option 2 or 3 for this feature as they provide the most flexibility going forward. But like I mentioned, I don't have the motivation to do this right now, even if I'd also like to have something like this ready for next league as well.

If you'd like to take a stab at this yourself, please be my guest as I'd very much welcome extra help on a hefty feature like this. If you do decide to try it, let me know if you have any questions or need any support on my end. Otherwise, maybe I'll have more motivation when GGG announces the details of the next league 🤣

r52 commented 4 years ago

Started prototyping a web app based solution. Watch out for the live-ui branch for WIP.

Rottenbeer commented 4 years ago

Hi I'm exited to see progress here :) How can I try it out? I compiled the newest version of PTA, also did npm run build and ./deploy/package.ps1 but I cannot find a hotkey to trigger the live view. Am I missing something?

r52 commented 4 years ago

The view opens using the usual Ctrl+D/Ctrl+Alt+D hotkeys. Make sure that in your PTA settings under UI, the template path is pointing to the index.html generated by npm build. Nodejs outputs the built files to ./PTA/search/dist/ which I then copy to templates/price/ in the build scripts.

Rottenbeer commented 4 years ago

Okay, I tried this and I only get a black window and the following logs: [2020-02-29T11:59:11] critical CONSOLE: Uncaught (in promise) TypeError: Cannot read property 'webChannelTransport' of undefined (file:///D:/git/PTA/PTA_x64/templates/price/js/app.87ec6441.js:1) - (unknown:0) [2020-02-29T11:59:16] debug Ignored/unprocessed line "Of ancient giants, none remain," - (unknown:0) [2020-02-29T11:59:16] debug Ignored/unprocessed line "Their only trace is timeless pain." - (unknown:0) [2020-02-29T11:59:19] critical CONSOLE: Uncaught (in promise) TypeError: Cannot read property 'webChannelTransport' of undefined (file:///D:/git/PTA/PTA_x64/templates/price/js/app.87ec6441.js:1) - (unknown:0)

Rottenbeer commented 4 years ago

I tried your CI artifact and it worked, so I guess it's a problem with my local setup. I compiled npm in a WSL, maybe I have to install npm on my windows pc

Rottenbeer commented 4 years ago

I really like the interactive version that you implemented, nice job! Would you please also think about the following:

Thanks a lot for implementing the feature!

r52 commented 4 years ago

FYI, getting undefined 'webChannelTransport' probably means you may have built your local copy of PTA using an unsupported Qt version, or the Qt files deployed in your build are out of date, or maybe a mix mash of both. The 'webChannelTransport' is provided by this function, and the QWebChannel javascript library built into the UI npm project used to receive that object only targets specific Qt versions. In this case, the library currently built-in target only Qt 5.12.6 and 5.12.7, so if you are building PTA on say Qt 5.14, the UI will currently fail to load.

The height of the Webview does not yet adapt to the content, so I need to manually resize it

Not sure what I can do about this one, or if I want to do anything at all, because the UI content is entirely reactive, so there is not really any way to resize a native viewport to match dynamically received content, which may or may not have different rendering sizes based on your OS configuration. Since the webview remembers your window size anyways, you can just resize it once to fit all of your search contents and it will retain that size afterwards.

It's no longer possible to quickly press Ctrl+Alt+D + Alt+ d to open the window and directly a search on pathofexile/trade and I really liked this in the native Qt version of the search GUI since it was possible to bulk search items in my farm tab very fast

This will depend on the possibility of adding hotkeys to the javascript side (I have no idea currently), so I'll take a look in due time.

The new search window does not open as fast as the old one, but the speed of the tool was one of the major selling points for me, maybe it can be increased with making the call to poe.prices optional or asynchronos?

Good point. The current method of executing the window opening isn't very optimized and needs to be refactored, but it currently 'works' lol. Adding an option to disable poeprices.info to reduce the load time is also a good idea to include, as not everyone wants this anyways. Keep in mind that the first spin-up of QtWebEngineProcess upon loading PTA is always slower, but hopefully the subsequent actions are fast enough.

A lot of this stuff pertaining to UI friendliness certainly makes me wish I had written this project in Electron instead, where UI components are much more responsive at the cost of persistent system resources and data processing speed. A part of choosing C++ was perhaps overcompensating for the sluggishness of PoE-TradeMacro, lol. But since PoE can't run on a potato nowadays anyways, maybe I'll even look into rewriting the app now 🤣.