Skip to content
This repository has been archived by the owner on Jul 15, 2024. It is now read-only.

Support timers on Wayland #150

Merged
merged 3 commits into from
Aug 16, 2023
Merged

Support timers on Wayland #150

merged 3 commits into from
Aug 16, 2023

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Aug 13, 2023

This was luckily really simple.

I've also added support for a blinking cursor to the edit_text example, to test out this functionality.

Reporting when/how to blink the cursor is a bit of an open question at the moment - it could even be the responsibility of the IME (via the input lock system?). But that can be improved later

TimeoutAction::Drop
},
)
.expect("could add a timer loop");
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I realize the API wasn't designed for it, but I think we should avoid panicking as much as possible. I sort of expect failures like this to be not really recoverable, but at least the application should be able to save data before bailing out...

Copy link
Member Author

Choose a reason for hiding this comment

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

We expected that we could - I believe that you expect what you expected, not what happened.

As far as I can tell you going through the control flow, this is definitely true anyway - i.e. could be an unwrap

Copy link
Contributor

@azymohliad azymohliad Aug 15, 2023

Choose a reason for hiding this comment

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

I believe that you expect what you expected, not what happened

My first reaction was: "aaah, that finally makes sense", I was always confused by the name expect. But then the actual message printed to the terminal would be confusing, because it doesn't put it in the context (such as "it was expected that ...").

Turns out there are guidelines on expect messages. So with "expect as precondition" style you write what you expected, but it still should be readable as a standalone error message. And I remain confused why the method is called expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

But yeah, if panic shouldn't actually be possible, I guess unwrap communicates it better to other people reading the code

Copy link
Member

Choose a reason for hiding this comment

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

Panicking as rarely as possible is something I strongly support in general. That said, I don't view it as a hard requirement for early work on features. So the panic could be removed during some later panic reduction work.

As for the panic message, none of our existing messages use this style. We mostly have descriptions of what failed, which I think is fine. We also have a few describing what should have happened. But yeah, none that describe what could happen instead.

I also don't really like the use of the keyword should as written in the guidelines, because it conflicts with the classic RFC definition of must vs should where should means actually optional. With expect it's clearly not optional.

As such I'd change it to either e.g. failed to add timer or timer adding must succeed.

Copy link

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

It works fine for me. Should some of those timer events have a lock acquired?

@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 15, 2023

I don't think so? Any locking is internal to the loop registration (and I don't believe we re-entrantly call app handlers within any registration methods)

@DJMcNab DJMcNab enabled auto-merge August 16, 2023 18:29
@DJMcNab DJMcNab added this pull request to the merge queue Aug 16, 2023
Merged via the queue into linebender:main with commit e7b4230 Aug 16, 2023
7 checks passed
@DJMcNab DJMcNab deleted the wayland_timer branch August 23, 2023 16:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants