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

implement select #45

Merged
merged 7 commits into from
Nov 10, 2023
Merged

implement select #45

merged 7 commits into from
Nov 10, 2023

Conversation

pm100
Copy link
Contributor

@pm100 pm100 commented Nov 5, 2023

second go.

tested with all crossterm/ratatui samples. All work, including search, default select highlight color changed to be different from search highlight. Tested with hard and soft tabs, mask mode. plain ascii and complex chars

Notes.

  • Changed delete_str core as delete_str_internal (user visible delete_str now calls delete_str_internal) to accept flags indicating whether or not to yank and / or delete. Since delete is optional maybe there is a better name. Why no delete sometimes? ctrlc / copy needs to yank but not delete. Why no yank? If I select some text and then type it overwrites the selected text, so I need to delete, but should not yank (IMO)
  • The main change for users of the API is the inclusion of shift as a modifier in Input, hence the tweaks to a few samples
  • The other change is if they do not use input or input_without_shortcuts they must call should_start_selection. I was wondering about adding a sample for that. None of the current samples need it but I know that the project I work (gitui) on would
  • added ctrl-x cut and ctrl-c copy
  • as far as I can see termion does not propagate the shift key so select does not work with it

@pm100 pm100 closed this Nov 6, 2023
@pm100 pm100 reopened this Nov 6, 2023
@pm100
Copy link
Contributor Author

pm100 commented Nov 6, 2023

also tested on mac and linux. crossterm / ratatui

termion on linux, ignores select shift key so select does not work, but works fine otherwise (Ie I have not broken termion support)

One issue so far, putty (ssh client) connection to linux does not propagate all keys states, in particular it doesnt know about shift.

tested on linux and windows using termwiz, works. Not exhaustively tested

@rhysd
Copy link
Owner

rhysd commented Nov 6, 2023

Thank you. I'll review this today or tomorrow. And adding tests is really helpful.

src/textarea.rs Outdated Show resolved Hide resolved
src/textarea.rs Show resolved Hide resolved
@rhysd
Copy link
Owner

rhysd commented Nov 6, 2023

One issue so far, putty (ssh client) connection to linux does not propagate all keys states, in particular it doesnt know about shift.

This sounds a problem of Putty and there seems nothing we can do.

termion on linux, ignores select shift key so select does not work

That's fine for now.

tests/textarea.rs Show resolved Hide resolved
@pm100
Copy link
Contributor Author

pm100 commented Nov 6, 2023

This sounds a problem of Putty and there seems nothing we can do.

I thought about for a few mins and this is a general issue with remote tings like SSH. They transmit the resulting ascii code not the actual key combinations.

  • 'a' sends 0x61
  • ctrl-a sends 0x01
  • shift-a sends 0x41

But it was good to test just to make sure something not broken (same as termion)

@pm100
Copy link
Contributor Author

pm100 commented Nov 7, 2023

i believe I have fixed all your issues. I also found a horrific set of bugs in multi byte char mode (japanese, etc). I did not realize that highlight boundaries are in bytes not chars. All fixed now

Copy link
Owner

@rhysd rhysd left a comment

Choose a reason for hiding this comment

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

This branch needs many refactorings but it is too time consuming for me to leave review comments on all places. And making the author fix all of them would be tough work. So I decided large refactoring after merging this PR.

For now, I merge this branch into main and then I'll add further changes tomorrow.

Thank you for adding this.

@rhysd rhysd merged commit 5a2649a into rhysd:main Nov 10, 2023
@pm100
Copy link
Contributor Author

pm100 commented Nov 21, 2023

@rhysd I am working on another big PR (for word wrap). I am trying to merge the current head and am surprised by how much you changed the select PR. Its up to the caller to control it entirely. The automatic stop and start worked perfectly in the pure input case. Its not clear now what one should do to minimal sample for example to make select work. I was not 100% happy with how it was controlled if you did not use the input function, but taking the auto select out entirely seems a bit harsh.

@rhysd
Copy link
Owner

rhysd commented Nov 21, 2023

@pm100 You can take 100% control of selection by TextArea::start_selection and TextArea::cancel_selection. TextArea::move_cursor doesn't change the selection state.

to make select work

What does 'work' here mean? What is not working on your local?

I am working on another big PR

Did you see this comment? Refactorying of internal structure is necessary to avoid adhoc changes. Large PR is not acceptable since it is too hard to make it maintenable source.

@pm100
Copy link
Contributor Author

pm100 commented Nov 21, 2023

@rhysd I misunderstood how you reworked the select PR. Ignore that comment, my bad

you are going to really hate the word wrap PR. But it adds huge benefit, please try it out before dismissing it. It was a lot of work !

@pm100
Copy link
Contributor Author

pm100 commented Nov 21, 2023 via email

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.

2 participants