Skip to content
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

RFE: Handling events in batches #422

Closed
walles opened this issue Jan 10, 2021 · 14 comments
Closed

RFE: Handling events in batches #422

walles opened this issue Jan 10, 2021 · 14 comments

Comments

@walles
Copy link

@walles walles commented Jan 10, 2021

Background

Looking at the demo application, a main loop is expected to look like this:

for {
	// Update screen
	s.Show()

	// Poll event
	ev := s.PollEvent()

	// Process event
	switch ev := ev.(type) {
		// ...
	}
}

The Problem

Now, let's say you get three events in sequence. With this pattern what will happen is:

  • Handle event 1
  • Redraw screen
  • Handle event 2
  • Redraw screen
  • Handle event 3
  • Redraw screen

Wish

What I would like to be able to do instead (in my case to get mouse scrolling to feel smoother) is:

  • Handle event 1
  • Handle event 2
  • Handle event 3
  • Redraw screen

Not sure what the best API would be here. Maybe this?

if !s.HasPendingEvents() {
	s.Show()
}

Or maybe this?

// Get multiple events in one go
events := s.PollEvents()

Either of these would require minimal changes to any app to get this performance improvement.

@walles walles changed the title RFE: PollEventNoWait() RFE: Handling events in batches Jan 10, 2021
@walles
Copy link
Author

@walles walles commented Jan 10, 2021

For some background, I just checked with Xcode Instruments while scrolling up and down using the mouse wheel, and:

So requesting fewer screen updates should help performance in this case.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 10, 2021

This seems like a reasonable idea. I think I'm kind of liking the latter option. We should probably also offer PostEvents() to be able to post multiple events at once too.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 10, 2021

Hm.... I wonder if it would be better (more idiomatic) to just expose the channel. Applications can then do select {} with a default case if they want to.

Sort of like this:

func Events() <-chan Event

This channel would return events, or nil when the application closes. Thoughts?

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 10, 2021

Actually....

PostEvents just posts to the same channel... so maybe that needs to be exposed as well, but have to think about application shutdown looks like.

@Bios-Marcel
Copy link
Contributor

@Bios-Marcel Bios-Marcel commented Jan 10, 2021

But if the channel just exposes a single element, isn't it useless, as you never know whether there's another one in the channel?

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 10, 2021

You can select with a default case.

func processEvent(ev tcell.Event) {
   if ev == nil {
     // application exited
    os.Exit(1)
  }
  switch ev := s.ev.(type) {
  // handle application events, but don't call show
  }
}

outer:
for {

   // wait for first event
  ev := <-s.Events()
  processEvent(ev)

  // and now look to see if there are more events...
  for {
    select {
    case ev = <-s.Events()
       processEvent(ev)
   default:
     // no more events
     s.Show()
    continue outer
  }
} 

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 10, 2021

Basically I'd have to code up something like the above loop anyway. This would allow applications to inject their own channels into the select, e.g. for network events, timeouts, etc. Which is kind of attractive to me.

@Bios-Marcel
Copy link
Contributor

@Bios-Marcel Bios-Marcel commented Jan 10, 2021

Ah, i thought you mean with events. But you mean returning arrays from the channel?

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 10, 2021

No. Read the sample code. Its a single event, but you keep getting them until there is nothing left in the channel. You get a notice that the channel is empty by hitting the default case.

@walles
Copy link
Author

@walles walles commented Jan 11, 2021

IMO exposing the channel might be more idiomatic.

My subjective opinion is though:

  • I do like the s.HasPendingEvents() API because:
    • It requires minimal changes to apps
    • It doesn't require any extra indentation of my event handling code
  • I don't like go's channel syntax, I think it's too wordy and nested.

Regarding injecting events, that can already be done using s.PostEvent(tcell.NewEventInterrupt(...)).

That said, what I really want is to be able to dodge some screen refreshes, and if I will be fine with any API allowing me to do that.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 11, 2021

If I expose the channel then you could just call len() on it.

@walles
Copy link
Author

@walles walles commented Jan 23, 2021

FWIW I just tried to work around this by accessing tscreen's evch through reflection and doing TryRecv() on it, but no luck.

Turns out TryRecv() doesn't work on unexported fields.

@gdamore
Copy link
Owner

@gdamore gdamore commented Jan 25, 2021

Exposing the event channel turns out to be a bit more complex than I'd like. The issue is dealing with close() of that channel, and handling how that works with respect to posting events.

Adding another layer of complexity is the fact that this event channel is a bit abused for multiple purposes. This is because I was rather naive about handling of multiple channels and select, and I just crammed everything into one channel when I first designed tcell. I'm a little more savvy (I think) these days, and really I should probably have split these into separate channels.

Stay tuned for more ... I have to be careful because I want to avoid breaking apps, which is my main concern.

@gdamore
Copy link
Owner

@gdamore gdamore commented May 16, 2021

I've elected the simple approach -- HasPendingEvent() bool -- it just returns true if len(s.evch) > 0

@gdamore gdamore closed this in 7a0b45c May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants