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

appctx: New event loop usage #87

Merged
merged 5 commits into from Jan 10, 2022
Merged

appctx: New event loop usage #87

merged 5 commits into from Jan 10, 2022

Conversation

LoganDark
Copy link
Contributor

The old model was hard to use and not very Rusty, so I decided to make it work a bit more like winit.

Also comes with a new demo showcasing how easy it is to use the new event loop to build a simple program.

The old model was hard to use and not very Rusty, so I decided to make it work a
bit more like winit.
Copy link
Collaborator

@LinusCDE LinusCDE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem reasonable and the example works fine.

Some points:

  • Add some text, telling you to draw would be cool. Got confused at first since nothing really happened (already had a blank screen though)
  • Address clippy and rustfmt failing.

I'm personally fine with the way this works. @bkirwi, @fenollp any thoughts here?

Screenshot:

@LoganDark
Copy link
Contributor Author

I've checked "allow edits by maintainers", feel free to make any changes you see fit. I think the rustfmt fail is because I use tabs instead of spaces - I'd rather leave the conversion to someone else, since I don't like spaces.

@LinusCDE
Copy link
Collaborator

Since there don't seem any additional comments from other maintainers, I'll just consider this change fine and merge it.

@LinusCDE LinusCDE merged commit 42fa2c8 into canselcik:master Jan 10, 2022
@LoganDark LoganDark deleted the event-loop-sanity branch January 10, 2022 16:45
@bkirwi
Copy link
Collaborator

bkirwi commented Jan 10, 2022

I personally don't have strong feelings about the change. I don't love the callbacks in appctx, and this looks like it might make it easier to ditch some of the awkward transmuteing down the road, but the new demo app seems to be about as easy to write if you avoided the appctx stuff entirely and just used the raw inputs and the framebuffer stuff directly. (Which IIUC is how most people are using the library anyways, including me!)

It's a bit awkward that libremarkable is a mix of low-level primitives for writing on the remarkable, plus some opinionated higher level stuff that many apps end up outgrowing. There may be more ways to tweak appctx such that it works well for more apps... but maybe it's worth fleshing out the documentation to make it more clear what parts are essential and which parts are easily ignored?

@LoganDark
Copy link
Contributor Author

LoganDark commented Jan 10, 2022

seems to be about as easy to write if you avoided the appctx stuff entirely and just used the raw inputs and the framebuffer stuff directly.

That's exactly what is happening except it's more difficult to set that stuff up without the appctx. appctx gives you events. Without it you have to start threads and stuff manually, so it's easier to just use appctx. You don't need to use anything except the event loop.

@bkirwi
Copy link
Collaborator

bkirwi commented Jan 10, 2022

Sure. For posterity, it's something like:

let (input_tx, input_rx) = channel::<InputEvent>();
EvDevContext::new(InputDevice::GPIO, input_tx.clone()).start();
EvDevContext::new(InputDevice::Multitouch, input_tx.clone()).start();
EvDevContext::new(InputDevice::Wacom, input_tx).start();
while let Ok(event) = input_rx.recv() {
    // ~whatever
}

(And drop some of the input devices if you don't care about those events, of course.)

appctx saves you a couple lines! But it's fairly close.

@LinusCDE LinusCDE added this to the 0.6.0 milestone Jan 17, 2022
@LinusCDE LinusCDE changed the title New event loop usage appctx: New event loop usage Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants