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.

Sign up for GitHub

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

Jump to bottom

FindReplaceOverlay/FindReplaceDialog: correcting regexp does not work #2657

Closed
jukzi opened this issue Jan 3, 2025 · 2 comments · Fixed by #2664
Closed

FindReplaceOverlay/FindReplaceDialog: correcting regexp does not work #2657

jukzi opened this issue Jan 3, 2025 · 2 comments · Fixed by #2664
Assignees
@HeikoKlare
Labels
bug Something isn't working

Comments

@jukzi
Copy link
Contributor

jukzi commented Jan 3, 2025

#2655 (comment)

having a java:

package a;

public class A {
public static void main(String[] args) {
	
}
public static void main2(String[] args) {
	
}
public static void main3(String[] args) {
	
}
}
  1. open search overlay
  2. enable regexp
  3. enter "main"
  4. press Search forward button
  5. enter "main\n" in Replace Dialog
  6. press Replace button -> does not work because of wrong line break (unituitive)
  7. enter "main\r\n" in Replace Dialog
  8. press Replace button -> still does not work WHY?
@jukzi jukzi added the bug Something isn't working label Jan 3, 2025
@HeikoKlare HeikoKlare mentioned this issue Jan 3, 2025
Merged
@HeikoKlare
Copy link
Contributor

I can confirm this issue. It is properly reproducibly with the given example workflow.

I can add that this issue has likely been there forever. It is reproducible in, e.g., a 2023-12 release and also a 2020-03 release (the latter with even more issues). Here you can see the same issue in the 2023-12 release:
findreplace_replace_regex_2023-12

The issue is rooted in the FindReplaceDocumentAdapter (or how its consumers use it). FindReplaceDocumentAdapter#findReplace(...) leaves an consistent state when not finishing successfully. This implementation and its consumers have not been changed for almost 20 years.
In my opinion, the produced state of that method should be considered incorrect (in contrast to requiring consumers to handle this behavior). I will provide an according fix proposal.

@HeikoKlare HeikoKlare self-assigned this Jan 6, 2025
HeikoKlare added a commit to HeikoKlare/eclipse.platform.ui that referenced this issue Jan 6, 2025
@HeikoKlare
Abort failing FindReplaceDocumentAdapter operations consistently ecli…
0353bb7 
…pse-platform#2657

The implementation of FindReplaceDocumentAdapter#findReplace(), used by
its API methods #find() and #replace(), currently leaves an inconsistent
state when failing because of an invalid input or state.
This inconsistent state affects the last executed operation type, which
the adapter stores in order to identify whether a replace operation is
preceded by an according find operation. In case the #findReplace()
method is called with an invalid position or to execute a replace
without a preceding find, the method aborts (throwing an exception)
without storing the operation to be executed as the last executed
operation, i.e., it leaves the adapter as if that method was never
called. In case the method is called in regular expression mode and the
expression used as find or replace input is invalid, the operation
aborts (throwing an exception) but still stores the operation to be
executed as the last executed operation, i.e., it leaves the adapter as
if that method was called successfully.

This behavior is unexpected as is handles invalid inputs inconsistently.
This also becomes visible in the existing consumers, such as
FindReplaceTarget#replaceSelection() used by the FindReplaceDialog and
FindReplaceOvleray. They assume that in case an exception occurs while
trying to perform a replace operation, a subsequent replace should
succeed without an additional find operation being required. This
assumption does currently not hold.

This change corrects the behavior of the
FindReplaceDocumentAdapter#findReplace() method to always leave the
adapter in an unmodified state when the method fails because of being
called with invalid input or in an inconsistent state.
In addition, regular expressions with an unfinished character escape at
the end now lead to a proper exception.

Fixes #eclipse-platform#2657
@HeikoKlare HeikoKlare mentioned this issue Jan 6, 2025
Merged
HeikoKlare added a commit to HeikoKlare/eclipse.platform.ui that referenced this issue Jan 6, 2025
@HeikoKlare
Abort failing FindReplaceDocumentAdapter operations consistently ecli…
f020fbe 
…pse-platform#2657

The implementation of FindReplaceDocumentAdapter#findReplace(), used by
its API methods #find() and #replace(), currently leaves an inconsistent
state when failing because of an invalid input or state.
This inconsistent state affects the last executed operation type, which
the adapter stores in order to identify whether a replace operation is
preceded by an according find operation. In case the #findReplace()
method is called with an invalid position or to execute a replace
without a preceding find, the method aborts (throwing an exception)
without storing the operation to be executed as the last executed
operation, i.e., it leaves the adapter as if that method was never
called. In case the method is called in regular expression mode and the
expression used as find or replace input is invalid, the operation
aborts (throwing an exception) but still stores the operation to be
executed as the last executed operation, i.e., it leaves the adapter as
if that method was called successfully.

This behavior is unexpected as is handles invalid inputs inconsistently.
This also becomes visible in the existing consumers, such as
FindReplaceTarget#replaceSelection() used by the FindReplaceDialog and
FindReplaceOvleray. They assume that in case an exception occurs while
trying to perform a replace operation, a subsequent replace should
succeed without an additional find operation being required. This
assumption does currently not hold.

This change corrects the behavior of the
FindReplaceDocumentAdapter#findReplace() method to always leave the
adapter in an unmodified state when the method fails because of being
called with invalid input or in an inconsistent state.
In addition, regular expressions with an unfinished character escape at
the end now lead to a proper exception.

Fixes #eclipse-platform#2657
HeikoKlare added a commit that referenced this issue Jan 6, 2025
@HeikoKlare
Abort failing FindReplaceDocumentAdapter operations consistently #2657
b448d7e 
The implementation of FindReplaceDocumentAdapter#findReplace(), used by
its API methods #find() and #replace(), currently leaves an inconsistent
state when failing because of an invalid input or state.
This inconsistent state affects the last executed operation type, which
the adapter stores in order to identify whether a replace operation is
preceded by an according find operation. In case the #findReplace()
method is called with an invalid position or to execute a replace
without a preceding find, the method aborts (throwing an exception)
without storing the operation to be executed as the last executed
operation, i.e., it leaves the adapter as if that method was never
called. In case the method is called in regular expression mode and the
expression used as find or replace input is invalid, the operation
aborts (throwing an exception) but still stores the operation to be
executed as the last executed operation, i.e., it leaves the adapter as
if that method was called successfully.

This behavior is unexpected as is handles invalid inputs inconsistently.
This also becomes visible in the existing consumers, such as
FindReplaceTarget#replaceSelection() used by the FindReplaceDialog and
FindReplaceOvleray. They assume that in case an exception occurs while
trying to perform a replace operation, a subsequent replace should
succeed without an additional find operation being required. This
assumption does currently not hold.

This change corrects the behavior of the
FindReplaceDocumentAdapter#findReplace() method to always leave the
adapter in an unmodified state when the method fails because of being
called with invalid input or in an inconsistent state.
In addition, regular expressions with an unfinished character escape at
the end now lead to a proper exception.

Fixes ##2657
@HeikoKlare
Copy link
Contributor

Fixed by #2664 (not auto-closed as convention was not met accidentally).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@HeikoKlare HeikoKlare

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@HeikoKlare @jukzi