Improved the select example.
#501
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now the `OnEventView.on_pre_event_inner()` calls return `Some(EventResult::Consumed(Some(Callback)))` instead of `Some(EventResult::Consumed(None))`. This follows the guidelines from documentation of methods returning a `Callback`, which say that it should be ran on the `Cursive`. While in this example this doesn't make a difference, the previous version created confusion for new users who might not realize you can pass the `Callback`s to the `Cursive` this way.
Owner
|
Hi, and thanks for the PR! |
TianyiShi2001
pushed a commit
to TianyiShi2001/cursive
that referenced
this pull request
Oct 9, 2020
Now the `OnEventView.on_pre_event_inner()` calls return `Some(EventResult::Consumed(Some(Callback)))` instead of `Some(EventResult::Consumed(None))`. This follows the guidelines from documentation of methods returning a `Callback`, which say that it should be ran on the `Cursive`. While in this example this doesn't make a difference, the previous version created confusion for new users who might not realize you can pass the `Callback`s to the `Cursive` this way.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
This PR is an effect of my initial confusion with the intended way of handling
Callbacks, and aims to help prevent that from happening to others in future.Story time!
I wanted to add custom keybindings to a
SelectView, so I looked through the examples and found theselectexample which looked like exactly what I needed. After replicating theOnEventViewwrapper part of the code, the keybindings worked as far as navigating theSelectViewwent, but I noticed all the callbacks set withSelectView.on_select()were ignored with the custom keybindings but worked with the default arrow ones.I started reading into the documentation, and noticed the
SelectView.select_up()method returned aCallback, and the documentation said it should be ran on theCursive. I couldn't figure out how to get a reference toCursiveinside the closure inOnEventView.on_event_inner()to run it like the documentation example has shown (something along the lines ofcallback(&mut siv)), so I tried usingOnEventView.on_event(), then getting a referece to theSelectViewby name, and usingCursive.cb_sink()to send theCallbacks but apparently they weren'tSyncso that didn't work out.Not until having found #433 and this comment in particular did I realize that I should look at the documentation of
EventResult, which, in turn, made me realize that I can just pass theCallbackinside theEventResult.Summary
I thought it would be a good idea to create this small PR to hopefully help other Cursive beginners be less confused :)
It effectively doesn't change anything in this example as far as functionality goes, but it helps bring attention to this important way of handling
Callbacks.