Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
x/exp/shiny: allow bounded types for Deque #20436
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:
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
Any feedback and/or thoughts for intention of Deque welcomed.
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.
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.
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
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.
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.