snowcap: add mouse_area widget#364
Conversation
|
Hello @Ottatop
The The issue with double_click is that two events are raised with the same target: on_press & on_double_click
An easy fix is to have the user define a unique identifier for the mouse_area
I'm also wondering if this should/could go into the protobuf definition, as a new message Key {
oneof key {
// autogenerated unstable
uint32 widget_id = 1;
// user defined, stable. Use if more than a single message can be generated at the same time
string unique_id = 2;
}
}Shifting it to the protobuf side means that both lua and rust can rely on a |
9979cdc to
1049269
Compare
|
Side note about the unique_id: It might be useful even for widget without callback. One of the reason I need the mouse area is to properly handle a clickable taglist for my bar. I'll likely use that value as an identifier for the state in the bar itself (I wrapped widgets so they have a view & update function), and that may prove useful for other stateful widgets, even without events driven by snowcap (e.g. a textbox). |
|
So the trick I was using in lua doesn't feel rust-y enough for my taste (memoize some value in a map/dictionary). I'm adding the unique_id to the GetWidgetEventsResponse type for the time being, but I really think it would be best to have it inside a |
da8af48 to
69c1148
Compare
|
On the second click, I'm not a fan of exposing a user-facing ID method and I think the current system of auto-generating IDs is good because we can hide backend implementation details like how widget IDs work. Exposing a unique_id method and requiring people to provide a unique ID is not ideal because we are a) pushing implementation details into the user-facing API which limits the ability to make breaking changes, and b) is error-prone. The current MouseArea API needs I'd do away with the unique ID and implement my suggestion above instead. This means we won't need the user to call a separate method to make things work.
I believe widget events were added after the most recent release so you don't have to worry about breaking changes in the development window since then. |
While I agree it's best to hide implementation details, this is causing some events to be dropped, even when batching events in GetWidgetEventsResponse (although that does fix the double click). The issue is that surface::update can be called more than once for a given view generation, but the client will always regenerate a view if at least on event (or event batch) is received. Added some log in snowcap_api::layer.rs to test this: Move event are sometime dropped: Scroll events are particularly bad: Here is the specific code I use for testing: apply the patch, then run: |
|
This is ultimately your call, but the way I see it:
I'm not a huge fan of Regardless, I think the event batching is a good idea :D I'm of course open to suggestion if you have a better idea. EDIT: it came to me that the issue might be solved by keeping the previous set of handlers, but that may consume more ram than simple strings. I'd also like to point out that iced has an opaque |
|
Agree that 1 is bad, I think any solution is required to process all events. With 2, 3, 4, and 5, while we stabilize the ids somewhat, we still run into the problem of sending widget events for stale views: For now the only way I've come up with to fix this to buffer all incoming input events while we're waiting for an update, then once it comes in we send all buffered inputs immediately and batch send the resulting widget events: If you have any ideas I'd love to hear them because all the current solutions have some downside or another. Or maybe the whole architecture is just flawed and we need to come up with something else. |
Ugh, I oversaw that ...
I feel this would be the correct way to fix the issue with the current architecture. I must admit I'm not sure exactly how to implement it as it is tho.
My 2 cents is that we're getting in edge-case of an edge-case territory. While it wouldn't be intuitive, I think the chances of triggering that bug are quite low. To have events happening on a future view would requires knowledge of how the view will look like.
I kinda like the idea of 'buffering inputs' (with the caveat I mentioned). As for an architecture change, the only alternative I currently see would be to have a more static view, where we don't implicitly call With this model, the client send a view only once and receive events as they happen, but the server is tasked to maintain the program state until the client explicitly invalidate the view (in which case it's ok to drop events). The issue with this model is that we loose on flexibility, and both the server-side and client-side become more complex because the server now has to handle the state logic until the view is dropped, and the client-side have to explicitly invalidate the view. EDIT: Thinking back on this, I'm not even sure it would solves the issue. You could still get a 'rollback' as the server would continue handling events while the new view is in-flight. I think as long as we have iced-rs on server-side, the best we can do is to keep the current model, with the input buffering you proposed. |
I did a quick & dirty implementation (I set a boolean when messages are sent to the client so that we wait the new view before processing any additional events). I'm not dropping events anymore as far as I can tell. I'll see if I can find any adverse side-effect (and fix the lua side, too), but I think we can roll with it, unless we find something major requiring a re-architecture. |
68d9cfa to
a43b384
Compare
|
I've re-added the lua side. As far as I can tell, input buffering works, and I don't see any bugs. I'll have to note that technically this is a leaky implementation if the config goes into an infinite loop in the Programs update function, because the event buffer is currently unbounded. It shouldn't be an issue as long as the config answer, because events are still batched together, so the whole event queue is flushed. We could switch the event buffer to a circular buffer, but I don't think it's actually needed (in the sense that if the config is stuck in an infinite loop, the user have bigger issue, so detecting that might be more valuable than using a circular buffer here). For this reason, I've not done that for the time being. Let me know if I should change the buffer to a circular buffer in this MR (this can also be added at a later point if we do encounter issues). @Ottatop let me know if the current implem works for you, and I'll rebase everything to cleanup the history before marking the PR as ready. -- testing patch with the lua & rust side: |
|
Ye we can roll with this impl. The event buffer being unbounded is fine, we can change it if it ever becomes an issue. |
a43b384 to
2f70b33
Compare
|
Uh I've found a slight issue that only affect winit. Mouse motion event's can get very chatty and can overwhelm iced_futures subscriptions channels (which have a size of 100 events AFAICT). If this happens, any further events in the batch are dropped which is less than ideal (if anything, I'd rather drop mouse events than other potentially more useful ones) A secondary issue is that the spammy nature of mouse motion events can lead to a bit of lag (that being said, I've compiled in debug, and I've added logging. Removing the logs themselves lead to fewer warning from iced, but the config taking too much time to handle events and send a view could cause events to accumulate). This is tied to the monitor/window framerate (main monitor refreshes I see a few possible mitigation:
In my opinion, if this is to be addressed, throttling motion events should be the way to go. |
I just want to clarify this. After further testing, I'm still unable to reproduce when pinnacle is started from tty (regardless of compilation option). I do think this can & should be mitigated by throttling mouse-move events (or any kind of chatty/repetitive hw events). Ideally it would be some dynamic adjustment to prevent hw events from overwhelming iced channels, while still allowing most events to go through. |
|
Yea let's push off figuring out throttling/other handling for spammy signals. This problem is probably going to affect other things like gesture binds in the future so I'd like to come up with a more general solution. |
2f70b33 to
519055c
Compare
71ee107 to
cbe17e7
Compare
|
Yes, PR for the event handling changes would be good |
cbe17e7 to
a6b27be
Compare
59abd52 to
f1c3582
Compare
7a9944b to
b3d063a
Compare
8222d94 to
2761894
Compare
067d766 to
d917f3a
Compare
d917f3a to
fc7b8d7
Compare
fc7b8d7 to
4b29c19
Compare
b460c35 to
608ddc7
Compare
608ddc7 to
96c5c6e
Compare
Ottatop
left a comment
There was a problem hiding this comment.
Yea no notes, looks good. Thanks!
This PR adds the MouseArea widget to snowcap.
This widget is a native Iced widget that allows finer control compared to Button, (e.g. knowing which mouse button has been pressed).