search

cyberjunk / meridian59-dotnet

New 3D client and tools for Meridian 59
GNU General Public License v3.0
34 stars 36 forks source link

Allow Invalidate to close specific ConfirmPopup windows. #299

Closed skittles1 closed 5 years ago

skittles1 commented 5 years ago

ConfirmPopup windows don't get closed on data invalidation (i.e. system save) which can cause an issue with stat reset - invalid stat reset data could be sent to the server even though the stat reset window itself has been hidden.

This change clears the data in ConfirmPopup and hides it if present when Invalidate is called and any open ConfirmPopup has the CloseOnInvalidate bool set to true. Currently set for stat reset, guild, quest and char create popups.

This change clears the data in ConfirmPopup and hides it if present when the visible status on the stat reset window changes from visible to hidden.

I think a more elegant way to handle this would be to inform the ConfirmPopup window directly from DataController's Invalidate method, maybe by having ConfirmPopup handle events on a new property in DataController (something like a DataInvalidated boolean that gets toggled at the start/end of Invalidate()). Let me know if you'd like something like that instead.

Fixes #296

cyberjunk commented 5 years ago

Reviewed this a bit.

What I don't really l ike is that it seems any open ConfirmPopup always gets closed as long as StatChangeWizward was "visible". Since StatChange wizard is not modal, I think a player can open the stat change wizard, and then cause a totally different popup, that should not get closed on serversaves/if statchange gets closed.

Regarding:

I think a more elegant way to handle this would be to inform the ConfirmPopup window directly from DataController's Invalidate method, maybe by having ConfirmPopup handle events on a new property in DataController (something like a DataInvalidated boolean that gets toggled at the start/end of Invalidate()). Let me know if you'd like something like that instead.

Possible, but there's another way to do this. The DataController in the core library is just the base class, the one actually used in OgreClient is "DataControllerOgre" in the OgreClient project. This already overrides a few methods, so you could simply make "Invalidate()" (or whichever you want) virtual in the base class and then override it with some additional code. From there you can also access the static UI such as the ConfirmPopup. But an event is also fine...

I think we might want to store a new boolean value with the confirmpopup, like "CloseOnInvalidate". This needs to be set by the ConfirmPopup user, like the others properties that get set. And a new method like "Invalidate()" that checks "CloseOnInvalidate" and if true, does things like your ClearData(). That Invalidate() would either be called from the event you mention or from the overriden Invalidate() of DataControllerOgre.

skittles1 commented 5 years ago

Thanks, not sure how I missed the DataControllerOgre class :) Will redo using the override.

skittles1 commented 5 years ago

Updated the PR description and commit with the Invalidate override method.

cyberjunk commented 5 years ago

Nice, that's exactly what I had in mind. Just curious why you changed the Invalidate() from public to protected, was that by purpose?

cyberjunk commented 5 years ago

Also, please check ControllerUI::IsInitialized in DataControllerOgre Invalidate() before you call DataInvalidated() on the ConfirmPopup UI window. Likely not a problem right now, but the DataController gets alive a lot earlier than the UI (and might also live during an engine/UI/window reload etc. when the UI is invalid)

cyberjunk commented 5 years ago

Going to merge and fix the minor things myself.

cyberjunk commented 5 years ago

See: https://github.com/cyberjunk/meridian59-dotnet/commit/94d40a7d7b3b20c350987b8cfc173054c6e51616