Implement markdown mutating checkboxes through CommonMarkViewer::show_mut
#40
Conversation
| @@ -252,6 +252,8 @@ struct CommonMarkOptions { | |||
| use_explicit_uri_scheme: bool, | |||
| default_implicit_uri_scheme: String, | |||
| alerts: AlertBundle, | |||
| /// Whether to present a mutable ui for things like checkboxes | |||
There was a problem hiding this comment.
Does this belong in CommonMarkOptions, or should it be somewhere else?
There was a problem hiding this comment.
It's fine for it to live there
| /// Shows rendered markdown, and allows the rendered ui to mutate the source text. | ||
| /// | ||
| /// The only currently implemented mutation is allowing checkboxes to be toggled through the ui. | ||
| pub fn show_mut( |
There was a problem hiding this comment.
You suggested show_mutable as the name, but I think show_mut is more idiomatic for a Rust API name.
| @@ -17,15 +19,15 @@ pub struct ScrollableCache { | |||
| /// Parse events until a desired end tag is reached or no more events are found. | |||
| /// This is needed for multiple events that must be rendered inside a single widget | |||
| fn delayed_events<'e>( | |||
| events: &mut impl Iterator<Item = (usize, pulldown_cmark::Event<'e>)>, | |||
| events: &mut impl Iterator<Item = (usize, (pulldown_cmark::Event<'e>, Range<usize>))>, | |||
There was a problem hiding this comment.
Maybe there could be a type alias for this, but I wasn't sure enough about it to make it one.
There was a problem hiding this comment.
I thought the exact same thing. I have some refactoring to do :)
| checkbox_events: Vec<CheckboxClickEvent>, | ||
| } | ||
|
|
||
| pub(crate) struct CheckboxClickEvent { |
There was a problem hiding this comment.
Not sure if there is a better name for this
| @@ -204,23 +215,25 @@ impl CommonMarkViewerInternal { | |||
| options: &CommonMarkOptions, | |||
| text: &str, | |||
| populate_split_points: bool, | |||
| ) -> egui::InnerResponse<()> { | |||
| ) -> (egui::InnerResponse<()>, Vec<CheckboxClickEvent>) { | |||
There was a problem hiding this comment.
It returns a Vec, but usually there is only one thing that the user clicks.
... Although there is multitouch and whatnot, so I think this is a safe option.
| @@ -325,25 +341,26 @@ impl CommonMarkViewerInternal { | |||
| scroll_cache.split_points.clear(); | |||
| } | |||
| } | |||
|
|
|||
| #[allow(clippy::too_many_arguments)] | |||
There was a problem hiding this comment.
Clippy complained about too many arguments.
Unsure if I could somehow reduce the number of arguments, so I just added an allow
| @@ -476,7 +494,19 @@ impl CommonMarkViewerInternal { | |||
| newline(ui); | |||
| } | |||
| pulldown_cmark::Event::TaskListMarker(mut checkbox) => { | |||
| ui.add(Checkbox::without_text(&mut checkbox)); | |||
| if options.mutable { | |||
There was a problem hiding this comment.
Immutable uses original checkbox widget that you made, while mutable uses normal egui checkbox widget.
| /// Shows rendered markdown, and allows the rendered ui to mutate the source text. | ||
| /// | ||
| /// The only currently implemented mutation is allowing checkboxes to be toggled through the ui. | ||
| pub fn show_mut( |
There was a problem hiding this comment.
| pub fn show_mut( | |
| #[cfg(feature = "pulldown_cmark")] | |
| pub fn show_mut( |
If you mark the entire function for the pulldown_cmark backend. Then you can remove the cfgs inside the function and that way it's obvious that it is not supported by the comrak backend.
There was a problem hiding this comment.
Good idea, I'll implement it
I'll try cooking something up EDIT: I added a |
|
Let me know if you need any other change. |
|
Thanks. This is great! |
Closes #38
This pull request neglects to implement the feature for the comrak backend. I haven't tested what happens when trying to use
show_mutwith the comrak backend.Since the comrak backend's future is unclear, this is acceptable.