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

Improved Regex handeling when searching #2373

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

jannisCode
Copy link
Contributor

@jannisCode jannisCode commented Oct 9, 2024

What it does

This PR improves the handeling of the Regex search when searching in the new or old find/replace dialog or the Big Search (strg+h). This is regarding #1204

Before

image

After

while typing:
Screenshot 2024-10-09 134416
when hovering:
Screenshot 2024-10-09 134409

Additionally to the functional changes the new class helps with reusing the code that handles the regex search.

@jannisCode jannisCode force-pushed the better_structure_six branch 2 times, most recently from 3e5dbbb to 93f7f83 Compare October 9, 2024 13:00
@@ -24,6 +24,7 @@
* The plug-in class for the org.eclipse.ui plug-in.
* This class is internal to the workbench and should not be
* referenced by clients.
* @since 3.207
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 40 to 45
*/

/**
*
* @since 3.207
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

@jannisCode thank you for the contribution. I made some comments regarding coding style and naming conventions.

I haven't tested it yet, I can do it once the PR is ready to be reviewed.

@jannisCode jannisCode force-pushed the better_structure_six branch 7 times, most recently from 3b530e1 to fe4eb3a Compare October 10, 2024 12:42
jukzi
jukzi previously requested changes Oct 10, 2024
Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

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

the screenshot looks nice

@jannisCode jannisCode force-pushed the better_structure_six branch 2 times, most recently from d00c824 to a6ff1a3 Compare October 10, 2024 14:37
@jannisCode jannisCode force-pushed the better_structure_six branch 3 times, most recently from e65493d to c50dfae Compare October 11, 2024 08:46
@jukzi jukzi dismissed their stale review October 11, 2024 08:54

changes applied

@jannisCode jannisCode marked this pull request as ready for review October 11, 2024 11:00
@jannisCode jannisCode force-pushed the better_structure_six branch 5 times, most recently from 08000ed to 1ce32d5 Compare October 11, 2024 12:46
@jannisCode jannisCode force-pushed the better_structure_six branch 3 times, most recently from d2e5a25 to 6651eaa Compare October 11, 2024 13:01
@Wittmaxi
Copy link
Contributor

This looks like a promising change. I wonder

  1. how easily can this be incorporated into the FindReplaceOverlay?
  2. how does this behave for multiline RegEx error messages?

It would be nice if you could provide 1) as a follow-up PR.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Looks better but I found one change in AbstractTreeViewer that doesn't belong here and I have another question regarding some removed invocations of evaluateFindReplaceStatus (which was renamed to decorate). Maybe the latter is the reason why @Wittmaxi asks for integration with the new find & replace overlay? I remember testing the previous version of this PR and the f&r overlay was working exactly as the other searches i.e. it was integrated before.

I haven't tested again, I will do it later today.

@fedejeanne
Copy link
Contributor

I haven't tested again, I will do it later today.

After testing, the behavior looks almost OK, I have only 1 finding (read further):

What works

The expression is validated in all 3 requested places:

image

image

image

The finding

It would be better if the f&r dialog deactivates all of the buttons below once the regex is found invalid, just like the Search dialog (Ctrl + h) does:

image

@Wittmaxi please test it too and let us know if you have any findings. Also, please elaborate on your request:

  1. how easily can this be incorporated into the FindReplaceOverlay?

I see the behavior already in it, maybe you meant something else by "incorporated" (it's a ambiguous term).

w.r.t. your other request:

  1. how does this behave for multiline RegEx error messages?

I also saw it and the solution is to "merely" adapting the (old) method SearchDecoration::getValidationError. Since I'm interested to know why it wasn't done like that in the first place (the method was already there, it was only moved in this PR) and there are some nuances with that request, I want to leave that for another Issue / PR --> vi-eclipse/Eclipse-Platform#130 (it's labeled with "Student" and it has no assignee yet so you may work on it too :-) )

@fedejeanne
Copy link
Contributor

@jannisCode there are conflicts that need to be resolved.

@fedejeanne
Copy link
Contributor

@jannisCode remove the "merge" commit

image

Better to rebase on top of origin/master when you want to bring the latest changes into your branch ;-)

Since the "committer" is GitHub, I assume it was done by clicking "Update Branch" in this PR?

@fedejeanne fedejeanne dismissed their stale review October 14, 2024 08:02

The changes in AbstractTreeViewer were reverted

@jannisCode jannisCode force-pushed the better_structure_six branch 2 times, most recently from 31fefe3 to b14bc4d Compare October 14, 2024 09:31
@fedejeanne
Copy link
Contributor

fedejeanne commented Oct 14, 2024

@jannisCode rebase again and then force-push your changes.

image

In the end, your commit (currently 31fefe3 but later it will be "renamed") should be on top of the commit 1eead54 (origin/master).

EDIT: you'll need to resolve conflicts when rebasing.

EDIT 2: you did it while I was writing the comment ✔️ ✔️ ✔️

@fedejeanne
Copy link
Contributor

The failing tests are unrelated --> eclipse-platform/eclipse.platform.swt#1518

Let's wait until the I-Build is done and re-trigger the checks to make sure nothing broke.

@jannisCode
Copy link
Contributor Author

#2373 (comment)
#2373 (comment)

done

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Ignoring random test failure #1941

@jannisCode please create a N&N entry

@fedejeanne fedejeanne merged commit 7587ea1 into eclipse-platform:master Oct 15, 2024
9 of 13 checks passed
@fedejeanne fedejeanne added the noteworthy Noteworthy feature label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
noteworthy Noteworthy feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants