x/exp/shiny: allow bounded types for Deque

[dskinner]

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

[nigeltao]

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.

[dskinner]

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.

[dskinner]

A case while working on animation with gomobile, last touch event (by Sequence id) to assure sync with painting. I find this case better represents the difficulties to address given the following as true:

• Animations (e.g. drag) may only be concerned with last input when firing epoch synchronously with paint.Event.
• A Paint program would not want any input discarded.

Of note is the relationship "last" events share with paint.Event; size.Event and touch.Event so far.

How important is "last"? That is, from my issue text, should N always equal 1?

For example, what if given a paint.Event, one always had access to the "last" of an event type based on position of paint.Event in deque? This doesn't immediately sound tenable given there's no bounds on event types that can be placed in deque, but I don't know. What if these relationships were defined explicitly? store last touch on paint and store last size on paint, etc. How much of the problem space does this solve?

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?

[dskinner]

A short followup on my June 2nd comment, I wasn't prepared to dive into anything outside Deque type, but your CL sparked some thoughts on my approach to this, as follows:

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 LastFoo methods is also demonstrative of app responsibility as the following would result:

• applications can still produce the original side effect.
• applications can choose not to produce the original side effect as demonstrated by my example program, ignoring LastFoo methods.
• applications can choose to use any combination of LastFoo methods available for an exponential set of possibilities.

This makes shiny's fault nothing more than being obtuse by exporting NextAction, where the average developer doesn't understand the consequence of calling this method, while another developer might understand the consequence of NextAction pulling from an infinitely buffered queue and see the possibility of congestion as self-evident.

To me, this is the problem to be addressed. Expanding paint.Event as mentioned in previous comment does not address this problem. That solution would have to be expanded further, such as isolating access to expanded paint events. So an ideal CL might have the following result:

• applications can no longer produce the original side effect, or the production of such is counter-intuitive.

[dskinner]

I'm now under the impression this idea of a lastEvent is code-smell and inclined to shift blame from application to shiny internals.

Animation example: translating an object on screen based on key input, common is to update a state object of translation intents. So if W and A keys are pressed, state object would have panUp: true, panLeft: true. During a paint event, one would then interpret this and update as follows:

• paint
• process translation events
• if any events processed, trigger repaint

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 size.Event is handled in shiny which is easily resolved by taking the same approach outlined above in animation example, but process size.Event before the first step, paint. Again, yes, this is handling the last size.Event before painting, but this should be expected behavior as well.

This only leaves one issue, a backlog of paint.Event. I don't have any immediate thoughts about how this should be addressed. There's already a note on the event type about checking if External but that's likely not sufficient.

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: https://github.com/dskinner/x/blob/3871b7b4aab8e8d5f402c317d2006a2a3be904b4/glw/example/glwidget/main.go#L141

Deferring layout: https://github.com/dskinner/x/blob/3871b7b4aab8e8d5f402c317d2006a2a3be904b4/glw/example/glwidget/main.go#L294

The main need of deferring layout comes about from synthesizing paint.Event on layout, again going back to the only real issue I see, a backlog of paint.Event.

[dskinner]

So in short, I'd enumerate the following issues to be addressed:

• resolve backlog of paint.Event
• update how size.Event works so it doesn't contribute to backlog of paint.Event

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:

• applications can no longer produce the original side effect, or the production of such is counter-intuitive.

[dskinner]

I'd also add, only handling the last size.Event is not a prerequisite of resolution, it's just the most immediate thought since it currently synthesizes a paint.Event that contributes to backlog.

Most layouts seem to run fast enough that it may be inconsequential to actually process every size.Event if it wasn't contributing to the paint.Event backlog.

[dskinner]

/cc @crawshaw you added External to paint.Event and reviewed previous CLs related, wondering if the above is sound.

Read from here for latest: https://github.com/golang/go/issues/20436#issuecomment-313265515 Read this comment for rationale one is to accept if exposing the last of an event: https://github.com/golang/go/issues/20436#issuecomment-312471328

[dskinner]

The following change seems to be sufficient:

diff --git a/shiny/widget/widget.go b/shiny/widget/widget.go
index 0dcbf63..943147a 100644
--- a/shiny/widget/widget.go
+++ b/shiny/widget/widget.go
@@ -111,6 +111,10 @@ func RunWindow(s screen.Screen, root node.Node, opts *RunWindowOptions) error {
                        root.OnInputEvent(e, image.Point{})

                case paint.Event:
+                       if e.External {
+                               root.Mark(node.MarkNeedsPaint)
+                               break
+                       }
                        ctx := &node.PaintContext{
                                Theme:  t,
                                Screen: s,

Thoughts? Motivation for this change is the question, why should there ever be more than one paint.Event in Deque?

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 size.Event synthesizes a paint.Event previously, please disregard.

[dskinner]

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:

some drivers require every paint event to be explicitly ack'ed

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 RunWindow implementations.

[nigeltao]

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.

[dskinner]

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.

[as]

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.