focustense / StardewUI

UI/widget library for Stardew modding
MIT License
6 stars 1 forks source link

ButtonPress events should short-circuit vanilla handling #49

Closed focustense closed 1 week ago

focustense commented 1 week ago

ButtonPress was an event added in the last release and although it looks the same as e.g. Click, it has some different lifecycle characteristics. Specifically, it is the only event that happens in receiveKeyPress and runs before delegation to vanilla logic.

For other events, we don't need a return value from InitiateXyz because it would never be used, but for ButtonPress, whether or not the event was handled can have an effect on later logic. One user was trying to prevent dismissal by hooking the menu button(s) and returning true from the handler, but it did not have the intended outcome of preventing the menu from being closed.

Simply, the ViewMenu needs to get the actual result from InitiateButtonPress and use it to return early if handled. A secondary question is whether the dispatch logic should run before the overlay-cancellation logic, as right now it runs after, and I can't remember if that was on purpose or arbitrary. Probably it should be able to short-circuit an overlay dismissal too, but I remember this being "touchy" when working on the keybind editor in particular, so it'll have to be tested for that.

focustense commented 1 week ago

Started looking at this but I'm not sure it's entirely useful/correct for the scenario outlined.

First of all, the actual gamepad button press has its own handler, which is skipped in the receiveKeyPress handler when it detects as being from gamepad. This means that, absent some hacks or more significant rework, we can only "suppress" keyboard keys (E/ESC), while gamepad B will still appear to ignore the outcome (since it's the key-press event that really handles it, as confusing as that is).

Second, and maybe more importantly, the button press event is only captured while the cursor is actually inside the menu's root view. With controller focus this is sort of workable, but with mouse movement it's just going to seem inconsistent and unreliable.

For the time being, it might be better to focus on some way to prevent menu dismissal at the actual menu level. Vanilla does have a way to deal with this explicitly (readyToClose()) which doesn't involve fussing over keys and input devices. The question is how to bind it with StarML, since we can't override the menu.

This really has to operate at or near the API level; at the view level we start running into complicated questions about included views, custom (add-on) views, different root types, etc. Passing a Func<bool> into the CreateMenu methods is one way, but as we discovered in the M3 release, it's actually not a backward-compatible change in Pintail-land even when the parameter is optional.

So the best alternative that comes to mind is to add a new interface to encapsulate any menu-level behavior, and the context type can implement that interface. Unfortunately, Pintail is not going to do all the work for us here either, because the interface isn't part of the API signature; the interface is really just a way to mark the context type and provide a bit of type safety on the modder's side, so we'd essentially have to do a modified duck-type conversion that involves looking for an interface with the same name, and mapping the properties/methods/etc.

Will see if a better idea comes to mind before starting on an implementation.

focustense commented 1 week ago

Closing in favor of #52.