New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/exp/shiny: allow bounded types for Deque #20436
Comments
I don't have a proposed solution, but there is a similar TODO in https://go-review.googlesource.com/c/18653/2/shiny/screen/screen.go and some discussion of the idea in the CL review at https://go-review.googlesource.com/c/18653/ If it wasn't mentioned then, there might also be an issue if some drivers require every paint event to be explicitly ack'ed. I can't remember if any of them actually do, but if we assume they don't, we probably need to document that somewhere. That CL hasn't been submitted. I don't think crawshaw and I came to an agreement, and fixing it hasn't been urgent. It has admittedly been sitting open for an embarassingly long time. |
I agree with David in the review, there haven't been enough cases; the issue sitting open is good. The infinite buffer is a good choice for baseline storage of events as a time-series. But when events leave Deque by calling NextAction, they retain this timing information (simply from being received serially) even when events in that time series share no commonality, e.g. what does painting a screen 120 times a second have to do with a key event to a developer? The only answer is a shiny/example that demonstrates synchronously receiving all event types, which happens to unintentionally be all examples and there-by acknowledge this as ok for all use cases. While a type switch is a common pattern and I'm sure made sense at one time for gomobile and shiny in receiving all events, consider it might be an error to conflate events that have no commonality and making shiny accommodate this might be inappropriate. Most importantly, an application has no choice but to produce this side effect, and so to respond after-the-fact when shiny is responsible for it before-the-fact is likely also an error and why I strongly disagree with the CL or making any association with WindowParameter types. And simply providing a generic solution (as in the text I filed for this issue) for flexibility is not, and should not, be the goal of whatever the proposed solution is. The goal should be addressing the next lifecycle stage of an event as it leaves the Deque so that a client application can receive without producing any side effects that shiny then needs to accommodate. Actual implementations will collapse down what I've broken out here. The CL for example has no literal before-the-fact and after-the-fact as discussed above that can be mapped on a time axis, but it has an implicit one and that's part of the problem. I'm still exploring the solution space. |
A case while working on animation with gomobile, last touch event (by
Of note is the relationship "last" events share with How important is "last"? That is, from my issue text, should N always equal 1? For example, what if given a If the last of an event is given importance only due to sync with painting, it would seem these should be coupled, instead of addressed without regard to this truth. So, is this true? |
A short followup on my Shiny is not responsible for the negative effect produced when resizing a window, the application is. This is given true by my example program where the effect is negligible. So, I also consider doing nothing at all a valid solution. Exporting
This makes shiny's fault nothing more than being obtuse by exporting To me, this is the problem to be addressed. Expanding
|
I'm now under the impression this idea of a Animation example: translating an object on screen based on key input, common is to update a state object of translation intents. So if
Yes, this is responding to the last key events, but this is expected behavior from an application stand point. I believe the confusion comes from how This only leaves one issue, a backlog of So, I don't think exposing the last of any event to the application developer makes any sense. I've updated my original example to reflect these changes. Handling of input: Deferring layout: The main need of deferring layout comes about from synthesizing |
So in short, I'd enumerate the following issues to be addressed:
This should happen in shiny. I'm otherwise not convinced anything more needs to be done. Resolving those two items would appear to target the goal of an ideal CL:
|
I'd also add, only handling the last Most layouts seem to run fast enough that it may be inconsequential to actually process every |
/cc @crawshaw you added Read from here for latest: #20436 (comment) |
The following change seems to be sufficient:
Thoughts? Motivation for this change is the question, why should there ever be more than one As far as I can tell, the only multiples would be external events as the Marks already address avoiding the issue. So, funnel external events into what's already addressing the issue. I was also in error saying |
bump, given this was around GopherCon before. I'm also under the assumption what I proposed in previous post doesn't resolve the concern @nigeltao raised with:
What platforms can that concern be tested today? e.g. OSX? I'm thinking, address this concern separately in shiny internals by determining what the minimum viable acknowledgement is to fulfill driver requirements that take little to no compute time to resolve. That might lead to better implementations of paint event propagation and |
Re the bump, I appreciate the intention to help, but as I've said elsewhere, I really don't have any spare time to devote to shiny in the forseeable future. That includes reviewing changes, not just writing my own changes. I can't speak definitively for @crawshaw, but I think that he is also low on shiny time. As for the "The following change seems to be sufficient" from June 6, my instinctive reaction is that it looks unusual to fix or work around this in shiny/widget instead of shiny/screen or shiny/driver. As e.g. the example/fluid app shows, it's totally legitimate to use shiny without using shiny/widget. If you really feel blocked on this issue, then I would suggest forking shiny. |
Thanks for the reply. A complete fix is going to require work in shiny/driver; this is perhaps a tedious separation of concerns. I'll wait for development to pick back up; there's enough low-level pieces exposed to push these concerns app-side. |
I do not have time to read the entire discussion right now, so my apologies in advance if this technique has already been mentioned. Ive experienced this deficiency in the deque too. One workaround I came up with was defining two events: Drain and DrainStop. When the event deque's contents were invalidated I would SendFirst() a Drain event and Send() a DrainStop event. The event loop would handle the Drain event by discarding all of the events until it recieved a DrainStop event. This worked very well for a graphical application that continued scrolling after the scrollwheel was released due to excess buffering. |
Calls to window.NextEvent() and the processing of the returned event all happen in the same goroutine. For paint and size events, this can cause Deque to congest with stale events.
Given the generic nature of type Deque, one method of addressing this is to allow declaring certain event types to be bounded, in that only N instances of the given type are ever allowed to exist and for simplicity let's say N=1.
In a shiny application, I only ever care about the last size and paint events, but I don't think it's appropriate to fuss over those details during any sort of iteration. If I declare a package level event, I might also want to tell Deque to only store N=1 instances.
To address this in an example program I wrote, I copy/pasted widget.RunWindow and inserted a small FIFO that exhausts Deque in a goroutine but only ever stores one instance of size and paint:
https://github.com/dskinner/x/blob/01a0257ede3a13250b22e5328097aa9d1a6911f5/glw/example/glwidget/main.go#L207
The above isn't done at great effort as the size and paint events only fire when que is empty. A second implementation tracked what would have been que insert index and on NextEvent, checked each bound type if insert index is zero or decremented, finally delivering an event from que if no bounded was sent (but this was just an example program so I removed this).
Providing some sort of
func (q *Deque) BoundType(T)
for the simple case of N=1 has a couple ramifications/questions:This is no longer exactly a "Deque" as we'd begin dividing into buckets. I don't know if there's a bucket (or otherwise) queue type out there that already addresses what I'm raising, but I'd like to research that or get feedback on that before committing to any sort of proposal.
For the specific case I'm suggesting above where only the last event of a given type is ever stored, it is, I suppose, theoretically possible to never receive that event given the right sequence of events if each event takes its rightful place based on its original insert index (and not replace the first instance which would negate this). Regardless, I don't feel it's appropriate to replace the first instance with the last and I'm not sure I really consider this case a practical concern.
Any feedback and/or thoughts for intention of Deque welcomed.
@nigeltao
The text was updated successfully, but these errors were encountered: