-
Notifications
You must be signed in to change notification settings - Fork 314
Add try_read
function
#1003
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
Add try_read
function
#1003
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally this seems like a good idea, I've needed or suggested to others something like this in Ratatui apps before, particularly for high frequency events like resizes that tend to come in batches.
As an aside - I have a refactoring PR that touches some close by code at https://github.com/crossterm-rs/crossterm/pull/999/files. Would you mind giving that a quick review? |
Yeah, for sure. I just reviewed that refactoring PR. I'll fix some of those bits you suggested in review in a bit. Thanks! |
@joshka I took care of some of the issues you mentioned in your review, let me know what you think. Also, don't stress about the commit mess here, I'll squash it before we merge the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in read are functionally equivalent to the exiting code (and neater). I've merged #999, which causes this to be in conflict now. Can you update this to hit the new code? Also a couple of minor changes for brevity
4dc1593
to
774f360
Compare
Hey, @joshka, I'm sorry, but I'm trying to squash all these commits down around the work that you've done and I think there has to be something that I'm doing wrong here. I didn't intend to make myself part of your refactor as a co-author, and these merge conflicts seem to keep popping up just about every time I squash two commits together with a rebase, which is the method I'm most familiar with. The branch I just force-pushed has several commits merged on top of commit I did a tiny bit of research and saw that I might have to reset and cherry-pick the commits to build a clean branch, and then force push it here, but I don't want to do anything before I know what you or other maintainers might think is the best approach. Again, I truly apologize for my lack of expertise here. I've never had to squash commits around incoming changes from master like this and I'm not really sure how to handle this. |
Don't stress - I'll make the changes and fix it up for you in a jiffy. |
Thank you! Out of curiosity, how did you squash those commits? Cherry-pick? |
No probs. You'll often find that it easier to squash first and then rebase if you're doing this with git (unless you learn the ways of I've just started using jujutsu though(https://jj-vcs.github.io/jj/latest/), so my process was a bit different. Something like: |
Closes #972
This PR adds a new function,
try_read
, that returnsSome(Event)
from the event queue if events are present, andNone
if otherwise. This could be used to process multiple events at once without using the blockingread
, which prevents the messy solution provided by @canac in their issue.