-
-
Notifications
You must be signed in to change notification settings - Fork 422
Add Typst integration (and appease clippy) #1587
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
base: main
Are you sure you want to change the base?
Conversation
|
|
I would appreciate feedback, especially from @flxzt, both about the looks/idea and the code |
|
I am unsure whether to increment the save version number, would need some info from the maintainers. |
background transparent by default
|
I think this is enough for a first version. The auto-save and option between pixel and vector are not must-haves, and about incrementing the version number, I would need feedback from maintainers (most of the save format seems to be from @flxzt) |
|
Not a maintainer, but looks pretty promising! I have a couple of comments / suggestions though (not in order of importance or anything): (also saw you just commit something as I'm writing this, I hope my comments aren't already out-of-date)
As for the UI
Finally, I'm still not too familiar with GTK and more complicated Rnote internals as a whole, so I can't comment on much more.. |
Thanks for the feedback, you are right, I did seem to make a few oversights. I will look into all of them, but I can already say that tinymist integration is close to impossible, at least for me. That is something that would require IDE-like infrastructure, which a simple GTK text field most likely does not have. |
|
There does seem to be a library GtkSourceView for gtk4 that helps with code completion, but still probably a herculean task to set up and integrate with something like tinymist. |
|
Yeah, that is something that is definitely too much for a first version, maybe at a later point in time. This is also not meant to be a Typst IDE, just a way to write something scientific like with LaTeX. |
|
Lilaq is now working |
|
I have now fixed all the feedback except for the misalignment of the middle line (which is draggable, and honestly not that bad), and the tinymist stuff since that is way too hard |
|
@flxzt I fixed all the clippy messages, if that was what is keeping you from merging this. Would be nice if you could merge this, since I have other changes conflicting with this I want to work on |
|
Typst support is something that is super exciting, this is quite incredible! I need to do a proper review on this huge diff. It looks good, but it might take a little while until I am able to do it |
Also, remove random vertical seperater from shape page
Vim habits caused me so much pain before this lol Also, I switched the position of the insert button
|
Btw if this ever gets merged, I also have another branch based on this where I also added a marker tool |
|
One trick that could be used for the IDE part is the use of temporary files. I've seen it done on some GUI sftp software. Show the folder content, and if you want to edit a text file on remote, you can "open with" your favorite IDE and it just opens the file with the IDE. Internally, it downloads and copy the file to a temp folder, calls the "open with" command on the file (so you can choose any program that opens txt), and more importantly watches the files so that when you save it in the IDE, it syncs the change back to the remote. Using the same tactic here, we could make it possible to edit each typst element in vscode with all the benefits of the IDE support |
|
I also noticed I need to set the flag for the flatpak somewhere so that it is allowed to access the internet for typst plugins |
This is not really something for the wrapper but rather for the file (you can change font color and background color in typst), but in the future, maybe it could put the color from the current tool as text color. I think that is a bit much for a first attempt though, as there are already huge changes.
See the conversation above, allowing 'esc' to exit would lead to the entire file being lost, which can be really annoying, especially for people frequently using vim where you
I noticed that too, however that is a bit hard to fix with the project structure. This happens because renderer and drawing are decoupled and the popup sometimes is not fast enough. For me, this only occurs the first time since once Typst is loaded, it is fast enough to open before I can click again, but that probably depends on system load |
|
Thanks for the really fast reply, Sorry for not entirely reading the conversation, I guess I was just too excited for the change! can't wait to see this in upstream and really looking forward to future features. |
Doublonmousse
left a comment
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.
For other comments on the thread
See the conversation above, allowing 'esc' to exit would lead to the entire file being lost, which can be really annoying, especially for people frequently using vim where you esc+:+w+enter to save a file. As mentioned above, maybe this could be solved by implementing an "open in editor", but since this is cross-platform, even supporting embedded, which I can't test, I am not sure if that is a good idea to add.
I'm not getting this (is it because you call update on the VectorImage on confirmation only ?). Do note that on the dialog itself, you can use https://world.pages.gitlab.gnome.org/Rust/libadwaita-rs/stable/latest/docs/libadwaita/struct.Dialog.html#method.connect_close_attempt or connect_close and do some additional processing before closing the dialog
- maybe set the text to be what's on the window
- and/or check if the text has changed, and if so force a new save/cancel dialog so you can't accidentally erase things
This is not really something for the wrapper but rather for the file (you can change font color and background color in typst), but in the future, maybe it could put the color from the current tool as text color. I think that is a bit much for a first attempt though, as there are already huge changes.
One idea is to get the background color from the settings and put this just below the typst code preview. That way it's not dependent on the light/dark mode, but whether or not the typst svg would be visible on a blank page.
| typst = { workspace = true, optional = true } | ||
| typst-kit = { workspace = true, optional = true } | ||
| typst-svg = { workspace = true, optional = true } | ||
|
|
||
| [dev-dependencies] | ||
| approx = { workspace = true } | ||
|
|
||
| [features] | ||
| cli = ["dep:clap"] | ||
| default = [] | ||
| ui = ["dep:gtk4"] | ||
| ui = ["dep:gtk4", "dep:typst", "dep:typst-kit", "dep:typst-svg"] |
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.
I don't think making typst optional works in that case.
Remember that the rnote-cli + the thumbnailer must be capable of rendering a rnote file to pdf/svg
|
|
||
| /// Insert an SVG image as a VectorImage stroke. | ||
| /// If `typst_source` is provided, it will be stored with the image for later editing. | ||
| pub fn insert_svg_image( |
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.
These functions feel a little bit out of place somehow. I think you could very well use import_generated_content with some adaptations (to not select the newly added svg)
| } | ||
|
|
||
| /// Update an existing Typst stroke with new SVG data and source code. | ||
| pub fn update_typst_stroke( |
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.
For this one, half of it could be a method in VectorImage like update_image and the rest that would stay would be related to the widget flags and store (if it doesn't come from the widget flags handler)
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.
Another thing on typing : I'm not sure the best way to go is to add a field to a VectorImage (which can be or not be from a typst edition).
Maybe it's a little better to add a separate typst type.
Note for compatibility : this PR needs to bump the rnote file version as the extra field makes it (probably) forward compatible (because of the serde skip if the typst source is one) but not backwards if a typst element is added
| if let Some(ref typst_source) = vectorimage.typst_source { | ||
| // Signal to UI that a Typst element was clicked for editing | ||
| *engine_view.clicked_typst_stroke = | ||
| Some((stroke_key, typst_source.clone())); |
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.
Why do we need the typst source here ?
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.
Maybe signaling could be done in handle_pen_event_up if you want only up events ? or select on down, act and reset on up ?
| } | ||
|
|
||
| fn today(&self, _offset: Option<i64>) -> Option<Datetime> { | ||
| Datetime::from_ymd(2024, 1, 1) |
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.
Do we care about the date ? Maybe a None (if we don't) or getting the date from the rust std to the datetime type would be better
| <property name="transition_type">GTK_STACK_TRANSITION_TYPE_SLIDE_LEFT_RIGHT</property> | ||
| <property name="interpolate_size">True</property> | ||
| <property name="hhomogeneous">False</property> | ||
| <property name="vhomogeneous">False</property> |
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.
I don't know if I prefer with or without the *homogeneous setting
| dialog.present(Some(appwindow)); | ||
| } | ||
|
|
||
| fn svg_to_texture(svg: &str) -> anyhow::Result<gdk4::Texture> { |
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.
There's probably everything you need in other functions.
There is an to_memtexture for the conversion from image to gdk4::MemoryTexture, and the VectorImage from_svg_str + gen_images for generating the image itself
| } | ||
|
|
||
| /// Takes the clicked Typst stroke if any, clearing it from the engine. | ||
| pub fn take_clicked_typst_stroke(&mut self) -> Option<(StrokeKey, String)> { |
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.
It's not great because you essentially take on each up event (which is a lot) vs doing a (is it some) and then taking
| pub camera: &'a mut Camera, | ||
| pub audioplayer: &'a mut Option<AudioPlayer>, | ||
| pub animation: &'a mut Animation, | ||
| pub clicked_typst_stroke: &'a mut Option<(StrokeKey, String)>, |
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.
I don't like this too much.
This is why you have double editors: in the time between the first click and the first editor popping up, any intermediary event can query another editor.
I want to do this another way
- set the fact that a typst stroke is selected in the widget flags
- Maybe keep only the strokekey here (and you may test for the presence of the typst source before pushing a Some in here, so Some for this means we selected a typst stroke)
- add a boolean property (either here, or preferably in the appwindow) to indicate whether or not an editor is opened (and to be safe this has to be large : set to true before querying the first editor, set to false after the editor is closed, and so that if it set to true no new editor is queried)
That way, the decision to open a new editor can be taken from the widget flags handler.
This is open to discussion as I'm not 100 % sure of everything here
|
Another though : you could store the selected typst stroke in the typewriter states so that signaling to open the editor is easier :
|





Fixes #644
Current state (as of first commit): button with a menu that has a preview button and renders into a vector
Ideal end state: button with a menu that automatically updates its preview, rendering into an image or vector format that contains its original text for editing, but can also cancel in cases where some resources went missing and can't be recovered