MissCorruption / QuickLootIE

Attempt at porting QLEE to NG with i4 compat
MIT License
7 stars 3 forks source link

Interface for integration - method registerOnTakedHandler #37

Closed AndbGame closed 1 week ago

AndbGame commented 1 month ago

WIP, not ready for merge. But Review will be nice

AndbGame commented 1 month ago

wow, thank you, for such a number of comments, I will definitely sort them out :)

AndbGame commented 3 weeks ago

in my opinion, I have taken into account all the comments, it is also not ready for Merge yet, I am in thought..., and I am in absolutely no hurry (especially in light of the ongoing refactoring and the lack of time :( ), but there maybe conceptual remarks for this

AndbGame commented 3 weeks ago

What am I thinking about now:

Implemented:

OnTakenHandler - Event fired after take some item from container in QuickLoot UI

        struct TakenEvent
        {
            RE::Actor* actor;
            Element* elements;
            std::size_t elementsCount;
            RE::TESObjectREFR* container;
            RE::TESForm* containerOwner;
            bool isStealAlarm;

                        // TODO: variant 1
                       Element* remainingEements;
               std::size_t remainingEementsCount;

                        // TODO: variant 2 (the container has become empty)
                       bool isLast;
        };

Looks I am need information about last taked element from container (Container is empty)

AndbGame commented 3 weeks ago

Additional 2 Api methods (I'm not sure yet, maybe I should reject some methods)

but there may be a very controversial solution that is easy to get around if you open the container in a native/game way... But we can also implement rejection mechanism(and api) for restrict opening container in a native/game way (but restriction for specific item and whole container - it is different things)


or canOpenHandler - Handlers can return kStop - Quickloot in this case can restrict show UI and opening container by native way

AndbGame commented 3 weeks ago
AtomCrafty commented 3 weeks ago

Regarding OnAttemptToTake: That sounds a lot like a concept commonly known as preview events. WPF nomenclature would be Take/PreviewTake, but other frameworks sometimes use Take/Taking, where Taking is the name of the preview event. The preview event handlers can cancel the whole operation.

I agree that these would be useful events to have:

I don't think we need to add customizable background colors, but the Select handler could provide data for the item info bar. The info bar is currently unused, but its purpose is to show additional information for the selected item in the blank space to the right of the weight info.

AndbGame commented 3 weeks ago

So, I have committed 3 events for now:

  1. TakenHandler - notify when and what user take from UI
  2. SelectHandler - notify when and what selected in UI
  3. LootMenuHandler (3 Status: OPEN, CLOSE, INVALIDATE): OPEN, CLOSE - notify when UI opening or closing, INVALIDATE - when itemList is changed (and which items is present in UI)

For now, looks such events is enough for simple integration (enough for my goals :) )

Conceptually, everything looks like it's quite workable, I've done the basic functionality of my addon, and everything seems to be OK, but now I want to focus more on it, maybe I'll find some problems

But the question of naming and additional refactoring not closed

Regarding OnAttemptToTake and other methods, which change behavior of QuickLoot - is more complicated - need additional refactoring in QuickLoot code

I do not completely refuse, but not in the current MR. (maybe after additional MR with refactoring/ some implementations). Changing the logic of the mod is a little more difficult task, I think, because it can cause bugs, so I'll postpone it for now.

Within the current MR, I will limit myself to simple events - notifications (maybe it is will be enought for first version of API)

AndbGame commented 3 weeks ago

LootMenuHandler (3 Status: OPEN, CLOSE, INVALIDATE): OPEN, CLOSE - notify when UI opening or closing, INVALIDATE - when itemList is changed (and which items is present in UI)

I think I've assigned too many responsibilities to one handler :)

AndbGame commented 2 weeks ago

At the moment, everything looks like it's ready.

An example of how I use it at the moment can be seen here: https://github.com/AndbGame/QuickLootIE-DD/blob/main/src/InterfaceQuickLootIE.cpp

Next step: I am waiting refactoring PR

MissCorruption commented 2 weeks ago

The refactor might take a long while, if you consider this feature complete and working, I'd merge it. Crafty mentioned that they'll adjust the rewrite accordingly. Assuming there are no issues upon review

AndbGame commented 1 week ago

@AtomCrafty Thank you for the work done! I am definitely merge it.

MissCorruption commented 1 week ago

Good job you two! I'm going to merge this then

AndbGame commented 1 week ago

@MissCorruption thanks! :)

Looks only question about thread safety has not been fully resolved. But also, by my mind - not have any problems with the current implementation

std::vector as I understand is thread safe for read

But:

  1. We can prevent adding new handlers (modify *Handlers) after kPostLoadGame/kNewGame
  2. Add lock guard for RegisterHandler

but it is unclear how appropriate this is, perhaps this is an excessive 'defense mechanism'