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

Add 'Skip to next (text) occurrence' feature to text editor #87883

Merged
1 commit merged into from
Mar 25, 2024

Conversation

TontonSancho
Copy link
Contributor

@TontonSancho TontonSancho commented Feb 2, 2024

Adds ui_text_skip_selection_for_next_occurrence action and related implementation to text editor. This action is bound to Ctrl+Alt+D shorcut.

Used in conjunction with ui_add_selection_for_next_occurrence, it gives the user the ability to select many occurrences of a selection and avoid some of them.
Used without a previous selection, the action jumps to the next occurrence of the current word under the caret.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

The behavior when you already have many occurrences selected and skip through an existing selection is a bit strange though. It's expected that these entries are deselected, but it cycles in an unusual order. In this video, I select all occurrences then skip through all occurrences:

simplescreenrecorder-2024-02-03_23.43.03.mp4

Nonetheless, it's not critical and I think having this is a big improvement already.

scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
tests/scene/test_text_edit.h Show resolved Hide resolved
@TontonSancho
Copy link
Contributor Author

TontonSancho commented Feb 4, 2024

The behavior when you already have many occurrences selected and skip through an existing selection is a bit strange though. It's expected that these entries are deselected, but it cycles in an unusual order. In this video, I select all occurrences then skip through all occurrences:

Yes, I totally caught that during my manual testing. It's mainly due to the first assumption done :

int caret = get_caret_count() - 1;

which gets the last created caret selection to do later stuff in the skip process.

I faced a lots of these "side effects" and this current (final'ish) behavior/implementation is a a trade off I made for this tiny ('n apparently) simple feature without completely the code base around it 😅.

I agree with you ('n thank you for that ❤️) the current state is acceptable.
It will be awesome if someone with UX skills could define these kind of feature in the future.

To forecast future implementation needs, It may be useful to have the following text_edit.cpp improvements :

  • FIFO caret storage
  • text search method returning position start and position end (will simplify new_caret / and new selection computation)

Regards

@akien-mga
Copy link
Member

@TontonSancho FYI, you marked some review comments as done, but there hasn't been a change to the PR yet. Maybe you're still working on it right now, but in case you thought this was updated already, it seems actually pushing your changes is missing.

Note: Please make modifications by amending the commit, and force pushing the changes, so that it stays as a single commit (see PR workflow).

Adds `ui_text_skip_selection_for_next_occurrence` action and related implementation to text editor.
This action is bound `Ctrl+Alt+D` shorcut.

Used in conjonction with `ui_add_skip_selection_for_next_occurrence`, it gives the user the ability to select many occurrences of a selection
and avoid some of them.
Used without a previous selection, the action jumps to the next occurrence of the current word under the caret.
@TontonSancho
Copy link
Contributor Author

@TontonSancho FYI, you marked some review comments as done, but there hasn't been a change to the PR yet. Maybe you're still working on it right now, but in case you thought this was updated already, it seems actually pushing your changes is missing.

^_^' It was building/testing on my side ...

Note: Please make modifications by amending the commit, and force pushing the changes, so that it stays as a single commit (see PR workflow).

I finally rebased and squashed the feature into a single commit.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 25, 2024
akien-mga added a commit that referenced this pull request Mar 25, 2024
Add 'Skip to next (text) occurrence' feature to text editor
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in d66539e Mar 25, 2024
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants