Text Selection for text_input widget

[FabianLars]

TL;DR Based on Finnerales fork, still WIP but looking for tips and/or wishes.

First of all, this is based on work done by @Finnerale (#184). It's the biggest and imo most difficult part of what has been done in this pull request (as of now).
My problem was, that I wasn't patient enough to wait any longer and i wanted this functionality for my side project. So although I'm an absolute beginner regarding Rust/GUI/Collab Work, I thought I'd give it a go.

What's implemented:

• Selection via Mouse dragging (by Finnerale)
• Ctrl + A or 3 clicks to select everything
• double click to select a word

What's left afaik:

• Shift + Left/Right = Expand selection by one character
• Ctrl + Shift + Left/Right = Expand selection by one "word"
• Shift + Home/End = Expand selection to Start/End
  (* I really need to work on Comments/Documention huh -> should be added tomorrow)

So most importantly I'd like to know if i did something wrong or inappropriate (regarding the code(style)) and if there is something i forgot to add.
Oh and also tell me if this is a waste of time for you (or me).

I'm gonna post this as a draft pull request, because it's obviously still WIP :)

P.S. Yea, i noticed the failing builds :P

[hecrj]

Thank you. I think this looks promising!

My main concern here is that the event logic is starting to become unwieldy. There is a lot of conditional logic and mutability happening in an uncontrolled manner (i.e. no opacity).

I think it may be a good idea to make the Cursor type completely opaque and move it to a nested module text_input::cursor and see what kind of issues we find by doing this. Cursor::simplify will disappear once we do this, as it won't be possible to build a Cursor externally. This type could also eventually be reused when we implement a multi-line text input (#197).

Also, it may be interesting to have a similar Interaction enum to hide the details of tracking double/triple clicks nicely.

In summary, I think we need to do a bit of thinking here:

• What are the most intuitive concepts that model the functionality that we are trying to implement?
• How do they relate with each other?
• What do they need to expose? What are their boundaries?

Answering these questions can help us leverage the type system to encode the requirements of our problem while reducing impossible states as much as possible.

[FabianLars]

Hey, thanks for your response.

To be honest that's already slightly over my head, but i really want to get it done, so I'll need a little time to really get into it (as I mentioned I don't have much experience). Especially the type opacity thingy is something I need to learn more about... (I'd appreciate any hints or something in the code base :P)

Btw, I'm also on Discord (+rust-lang server) and your Zulip server, if you want to reach out to me somewhere else.

[hecrj]

To be honest that's already slightly over my head, but i really want to get it done, so I'll need a little time to really get into it

Sorry, I didn't mean to overwhelm you! There is no rush. I think we have done the easy part here, which is code. The hard part is figuring out how the new code fits with the older code: the design.

This is the hardest part because we need to be able to see the big picture and the details of the problem at the same time. However, the changes here are very local, which should make our lives easier when it comes to simplifying things.

Especially the type opacity thingy is something I need to learn more about

Think about it like encapsulation. A type where all its internals are opaque and can only be built and mutated in a controlled manner through its exposed methods. Ideally, these methods should be meaningful (no setters!) and intuitive, representing clear use cases (move_left, move_right, select...). In case you may find it useful, I wrote an article about opaque types in Elm.

(I'd appreciate any hints or something in the code base :P)

#95 is a good example of how some logic can be simplified by introducing an additional type. I think something similar may apply here. It is hard to give you any more hints because every problem is unique! Exploration is part of the solution.

---

I know I am very nitpicky when it comes to design and simplicity. But I really believe that investing extra effort and time in making things simple is worth it in the long run.

I am also aware that I may be asking too much, so no pressure! This is already great! I should be able to eventually take it to the finish line. It may take some time, though, as there are other features and PRs I want to tackle first.

That said, if you still want to give it a shot, I will be glad to help and discuss the details on Discord or Zulip. In fact, this is the first thing I encourage contributors to do!

[luleyleo]

I currently don't have the time to work on it, so thanks for continuing this 👍

It might be interesting to take a look at how druid deals with cursors and editable text, as they did it in a way more abstract / general approach than I did.

Especially it could be worth to allow backing text edit components by something like ropey as it would likely perform much better than a String for long text.

[FabianLars]

So this all more or less happened while trying out different stuff to further get into this part of the codebase and the stuff you told me about, so it's still far from done (more or less the same features as previous commit but i guess it needs a huge cleanup), but I think it's already better than the previous version.

My question is if this approach is any better in your eyes any (or actually somewhat worth doing).

(And yes, i wanted to reach out to you about it before investing that much time, but i just got lost or sth, idk ¯_(ツ)_/¯ )

[yaskhan]

Maby add some shortcut keys util class? Like: https://docs.sencha.com/extjs/6.5.3/classic/Ext.util.KeyMap.html

[hecrj]

@FabianLars This is great! It's exactly what I wanted you to try.

By splitting cursor logic into its own type, we have a better understanding of the relationship between the different parts involved: TextInput, Cursor, and Value. The hard part is mostly done! A lot of patterns are very apparent now. We are close to modelling this very nicely!

I left some thoughts and ideas that I believe should improve the code and API considerably. Let me know what you think.

[FabianLars]

I played around with the things you suggested, but left out the Editor struct for now (or at least for this iteration). -> See Commit Message

let me know if you see something standing out. (I really need to work on my english skills lol)

Thanks for being so patient with me btw :)

P.S. I just noticed that i left the double/triple click functionality broken somehow

[FabianLars]

I changed it from Draft to normal pull request because I implemented the last missing select actions (tell me if there are more), except for copying into Clipboard but as far as i can see this is impossible with current Clipboard lib? (edit: i just ran cargo update and it mentioned a clipboard repo so i'll look into it again) (edit2: nevermind)

I'll see if i can clean up things further and finally take a look at your Editor suggestion if still needed (wanted to implement the missing keyboard actions first to see what the Editor needs to handle).

[hecrj]

No worries! I think I can take it to the finish line from here.

I will let you review the changes once I get a chance to work on this.

Thank you for all the work so far!

[FabianLars]

Thank you for actually giving me a chance to do this and helping me along the way! If you change your mind and see something you want me to do for this pr, just lemme know. (if this would be the editor thingy, we had to talk some more in depth about it i guess :P)

I hope there's something more to do for me in the future. (Hopefully I'm a more experienced rust user by then, so it'll be easier to work with me :P)

P.S. I'll push a really small commit with changes i forgot.

[Restioson]

An assertion fails if the value passed to the textbox doesn't change and some text is typed. See this example. After uncommenting the indicated line, it works as expected. The panic is as follows:

thread 'main' panicked at 'assertion failed: index <= len', <::core::macros::panic macros>:3:10
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
...
  15: iced_native::widget::text_input::Value::insert
             at native/src/widget/text_input.rs:769
  16: <iced_native::widget::text_input::TextInput<Message,Renderer> as iced_native::widget::Widget<Message,Renderer>>::on_event
             at native/src/widget/text_input.rs:327
  17: <iced_native::widget::column::Column<Message,Renderer> as iced_native::widget::Widget<Message,Renderer>>::on_event::{{closure}}
             at native/src/widget/column.rs:161

---

After looking into it a bit more, the issue seems to stem from the assumption that characters get added so the insert(new_char, cursor_pos) works (since cursor_pos is incremented regardless of insertion). Thus, on the second increment, cursor_pos is 1, so the insert is out of bounds.

I'm not sure if this should be considered a bug, but it should probably be noted in the documentation that the value must be updated after a message is sent. Maybe like so:

     /// It expects:
     /// - some [`State`]
     /// - a placeholder
     /// - the current value
-    /// - a function that produces a message when the [`TextInput`] changes
+    //// - a function that produces a message when the [`TextInput`] changes and updates
+           the value accordingly. If it does not update the value, the widget can panic.

However, this behaviour may pose additional challenges for programatically setting the value (as this wouldn't update the cursor). One way to design this would be to force the user to correctly update the cursor as well when updating the value. The issue for both of the above solutions would be that it would be non-intuitive (unless you RTFM, which people rarely do). This, however, may be the best option. I don't know.

[Restioson]

Found another issue (definitely bug) with the same example (two for one, heh). If you type a character and press backspace twice (or two and backspace twice), it will panic with attempted to subtract with overflow:

The relevant code appears to be here (text_input.rs:343):

keyboard::KeyCode::Backspace => {
    match self.state.cursor.selection_position() {
        ...
        None => {
            if self.state.cursor.start() > 0 {
                self.state.cursor.move_left();
                let _ = self
                    .value
                    .remove(self.state.cursor.start() - 1); // <--- HERE
            }
        }
    }
    ...
}

I think that that that line should be changed to .remove(self.state.cursor.start() - 1), as the move_left will already remove 1 from the start. Changing this line to that appears to fix the problem. I'm not sure if it causes any other side effects though.

[FabianLars]

Hey, thanks for reporting these bugs! I'll push 2 small changes which should fix these issues, hoping that hecrj didnt work on this pr locally already lol

[Restioson]

Np. What do you think about the first issue (not a bug per se) in terms of design/API?

[FabianLars]

Well, the crashes in 1) itself are fixed with these changes (although there might be a better solution), so that it can't panic if you're not updating the value. This should more or less represent the behaviour in the current master. And manually updating values after a Message is a requirement for every widget, not just for text_input.

Edit: Oh well, you meant the last part about programatically setting the value via self.value = "blabla".to_string(), huh? Then the cursor would be at 0 or 1 or something, which would be kinda meh indeed, but I'm not sure if there are actually use cases for this where you would update the value this way while it's focused/being used.
Tbh, I'll just wait for hecrj's opinion :P

P.S. Oh man, I'm too tired to write understandable sentences in english D:

[hecrj]

The value provided to a TextInput can change at any time. There is no consistency guarantee that we can rely on. The State should be completely fault-tolerant and never panic, independently of what the user/application code does.

Therefore, clamping with min and max is the way to go here.

[FabianLars]

Absolutely, that's what the commit was about (although the pr might need more improvements regarding this). So it shouldn't panic anymore but the cursor behaves not as expected (from the users POV) when you change the value programatically in the apps update function. But I'm not sure if that's really needed and if so how to implement a fix.

I hope I actually understood both of you correctly...

[hecrj]

No need to worry about that use case for the time being, although I would expect the cursor position to stay in the same place even if the value of the input changes. Isn't this how it behaves currently?

[FabianLars]

Yes, that's the current behaviour. But with the last commit if you start with an empty input and later change it, the cursor might be at position 1 instead of 0 because of the way I implement the fix. (If you do what caused the program to panic before == typing into an input that doesn't update its value)

[hecrj]

Thank you @FabianLars and @Finnerale for all the work here! This is great.

I have taken a final look and made some changes. Specifically, I have:

• Refactored the previous mouse::State and converted it into a simple Click type. A Click can be created with a position and an optional previous Click instance.
• Changed the Cursor API to require a Value in most of its methods. This makes boundary checks unavoidable, increasing safety.
• Implemented a draft of the Editor idea we talked about. I think it encapsulates editing logic quite nicely and could be expanded in the future.
• Tuned the renderer to avoid rendering the cursor when a selection is active.
• Added documentation for all the new stuff.

And I believe that's it! I am quite happy with the current state of this. Let me know what you think and we can merge it!

[FabianLars]

Looks great!
I have nothing to add at this point, so I'm ready to see it getting merged.

Most commits seem pretty straight forward (or better: were easily understandable to me). This should help me doing better PRs in the future.

And the editor implementation looks good and actually way easier than I expected it to look like :D

[hecrj]

Awesome! Let's merge this! Thanks again, everyone. 🍻