aRTy42 / POE-ItemInfo

Item Info Script for Path of Exile
166 stars 224 forks source link

Added connected map info to the dungeon. #71

Closed paulcdejean closed 7 years ago

Eruyome commented 7 years ago

I have nothing against the feature but I don't like the implementation, for several reasons.

  1. Having to manually edit a text file is a no go.
  2. I use ItemInfo included in TradeMacro, so I dislike (not completely disapprove) adding hotkeys, there are enough already. I always look for other solutions first.
  3. ItemInfo doesn't really add a custom hotkey to the script, it simply overrides PoE's ctrl +c hotkey. Custom hotkeys should only be added via ini/settings menu, never by editing the script. This is not implemented yet in ItemInfo, only in TradeMacro. So we would have to move the TradeMacro code to a lib function and also use it in ItemInfo. Then in TradeMacro we would also have to make sure that there are no duplicate hotkeys between ItemInfo and TradeMacro.
  4. I read the reddit thread and the mentioned suggestions. Having a popup asking you if you want to run this map, caching it until it's unlocked is also a no go. It's way to prone to errors, since this caching can be interrupted was too easily. A popup in general (even to directly add it) could be disruptive for TradeMacro price checking, depending on the implementation, so we have to be careful here.
  5. The user map list has to be saved in the user settings folder, not the macro folder!

So, lets talk about some solutions. I haven't thought this through very much so these are just from the top of my head.

  1. Add a GUI (system tray -> context menu) that shows all maps with checkboxes to lock/unlock them and buttons to mark all as unlocked or reset them.
  2. Now it could be inconvenient to open this GUI everytime to add a map. So either it is a bit inconvenient or we add a hotkey, but I'd like to avoid it.
paulcdejean commented 7 years ago

Oh weird I didn't think it would combine all the commits. I mean to just PR the first one.