Skip to content
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

Floem editor #296

Merged
merged 23 commits into from
Feb 7, 2024
Merged

Floem editor #296

merged 23 commits into from
Feb 7, 2024

Conversation

MinusGix
Copy link
Member

@MinusGix MinusGix commented Jan 28, 2024

This PR breaks the core editor portion of Lapce out into a separate module.
The editor being a separate module allows it to be used by other programs that utilize Floem, allowing us to share the improvements (and testing, which this makes simpler) between the two.
The basic idea is that it should be extensible (or, replaceable, like copying & pasting with modifications the view creation functions) enough to use in Lapce without too much work, while also not being very noisy with extension points that are painful to maintain stability.

This was initially developed as a crate in the Lapce repo, but discussion with dzhou ended up with deciding on putting it in floem (under a feature flag). See lapce/lapce#2909 for the Lapce PR which has most of the git history as well.


The primary trait is Document. It provides text (the xi-rope Rope), cache revision, IME preedit, screen lines, running commands, and some more.
Lapce's Document (now named Doc to avoid name collisions) would impl this trait, and be pretty close to what it once was.
Of note is that unlike the original Lapce Document the doc does not handle text layout creation directly, merely specifying how the text layout should be styled.

The movement logic supports modal mode movement, as I don't see a good way to cleanly separate them, but the default keymap does not support modal mode.

An Editor is in a way the data for the view into a document. Each distinct input box is a different Editor (data) + EditorView (view). Multiple editors can use the same document, automatically mirroring changes across them while allowing different styling or altered behavior in the other editor.


Room for improvement:

  • We could have a default context menu, or have it be built by Styling.
  • Put strum under a feature since we only really have that for Lapce's benefit?
  • Lapce had editors created with a passed in an Rc<Doc> which the constructor then turns into an RwSignal<Rc<Doc>>, does that actually provide any benefit now? Can we just swap that out with having every editor into a doc sharing the RwSignal rather than just the Rc?
  • Preedit is on Document because we want it shared between different editors. However it would be nice to make it automatically handled somehow?
  • We need single line text inputs. Lapce already has some of these, but I don't believe they're as customizable as the current floem text inputs.
    • Ex: how do we do password-like inputs without replacing a core part of the text layout drawing?
      • Simple solution: add a bool signal for whether to draw text, have password inputs set that to true by default and draw circles for text.len()
  • Various todos littered about
  • Needs testing & benchmarking, which this makes easier due to more separation of logic
  • Could we avoid depending on xi-rope for simple applications? Simple inputs and small textareas don't need as efficient operations.
    • Making everything generic over a type implementing RopeText would work, but be very noisy.
    • Having a Rc<dyn RopeText> (after modifying RopeText to be object-safe) would work, though I think we should worry about how much some logic directly benefits from inlining multiple operations done to the same text!
  • Is there any decent way we could have RwSignal<dyn Document> which is then downcasted to RwSignal<Doc>, because the caller wants to use their specific type?
  • Probably things I partially broke and just haven't noticed
  • Remove downcast-rs once the commented issue is stabilized
    • Possibly put it under a feature flag, because you only need it if you want to get access to the underlying doc which isn't always needed
    • Is there a different alternative that is more common which would work, esp. if we already transitively depend on it
    • We already depend on downcast-rs on linux via wayland-backend, so /shrug
  • Alternate formulation for SimpleStyleBuilder?
    • I made this a separate structure because some of the function names would be the same as on the Styling trait. However we could just say 'whatever' and have name collisions like that since ~most users aren't going to have the Styling trait in the same scope.
  • Might be a good idea to have some way for documents to know when editors destroy themselves? Like TextDocument has editor-specific command listeners

@MinusGix MinusGix force-pushed the floem-editor branch 2 times, most recently from a284c65 to bd5ac75 Compare January 29, 2024 14:11
src/editor/gutter.rs Outdated Show resolved Hide resolved
@dparnell
Copy link
Contributor

dparnell commented Feb 3, 2024

Hi, I have packaged this view up into a standalone repo here: https://github.com/dparnell/floem-editor-view/


impl MoveCommand {
pub fn to_movement(&self, count: Option<usize>) -> Movement {
use MoveCommand::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just reading through the code and noticed this. Not necessarily super important.

Using use enum::* can be a pretty bad idea.

If we were to ever remove one of the variants from MoveCommand it wouldn't cause a compiler error and that variant would then act as a catch all that matches all other patterns. So then if another command was added it would be matched under that variant.

It's generally better to do use MoveCommand as MC or some other short name. This adds the convenience of the short name and doesn't cause the same issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be caught by Rust warnings because it would be unused.
(Minor, but this is how it is done in Lapce)

@MinusGix
Copy link
Member Author

MinusGix commented Feb 7, 2024

Also I don't think clippy is actually running on most/all of the added code because it is under a non-default feature. Probably the CI / Rust isn't building with the features either.

@dzhou121
Copy link
Contributor

dzhou121 commented Feb 7, 2024

Also I don't think clippy is actually running on most/all of the added code because it is under a non-default feature. Probably the CI / Rust isn't building with the features either.

Maybe add editor to default features?

@MinusGix
Copy link
Member Author

MinusGix commented Feb 7, 2024

I think there's some benefit to having it non-default, as it makes so widget-crates won't accidentally leave it enabled, but also text inputs are pretty common, so I'll just add it to default features.
(I do think we should fix the CI to compile with all features enabled to avoid this in the future though, I'll open an issue)

@dzhou121 dzhou121 merged commit d9a3fb8 into lapce:main Feb 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants