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

Multi line error message fixed #2407

Merged

Conversation

jannisCode
Copy link
Contributor

What it does

This PR solves the issue from here: (vi-eclipse/Eclipse-Platform#130 (comment))

Before

The error message did not handle the different widths of characters well. Especially the ^ was on the wrong charachter, basically making it useless.
image

After

Now the ^ is on the right character, no matter what the user types in.
image

Copy link
Contributor

github-actions bot commented Oct 15, 2024

Test Results

 1 820 files   -  1   1 820 suites   - 1   1h 52m 37s ⏱️ + 4m 39s
 7 714 tests ± 0   7 484 ✅  -  2  228 💤 ± 0  2 ❌ +2 
24 258 runs   - 45  23 519 ✅  - 37  737 💤  - 10  2 ❌ +2 

For more details on these failures, see this check.

Results for commit 506587a. ± Comparison against base commit 4fbbe1b.

♻️ This comment has been updated with latest results.

@jannisCode jannisCode force-pushed the multi_line_error_message_four branch 5 times, most recently from 31f1538 to 822cb36 Compare October 21, 2024 10:56
@fedejeanne fedejeanne marked this pull request as ready for review October 21, 2024 11:54
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.

I have some minor comments regarding the coding style.

I also tested it and sometimes the ^ is a bit off.

Regex: [ --> ✔️
image

Regex: [d --> ✔️
image

Regex: [df --> ❌ (a bit to the right)
image

Maybe you can work with Hair spaces or Thin spaces?

@Wittmaxi
Copy link
Contributor

I think it is important to mention here that this PR relies very heavily on the RegEx error message format.
Any changes to that format or any errors on the compiler side would fail silently

https://docs.oracle.com/javase/8/docs/api/java/util/regex/PatternSyntaxException.html#getIndex--
https://docs.oracle.com/javase/8/docs/api/java/util/regex/PatternSyntaxException.html#getPattern--

Java DOES have a method that helps with building your own error message - the catch: the FindReplaceTargetStatus object does not provide them. It should be rather easy to add those into the object and to modify the message generator if required.

@Wittmaxi
Copy link
Contributor

Modify in org.eclipse.ui.internal.findandreplace.status.InvalidRegExStatus

	public int getIndex() {
		return regExException.getIndex();
	}

	public String getPattern() {
		return regExException.getPattern();
	}

@jannisCode jannisCode force-pushed the multi_line_error_message_four branch 4 times, most recently from 439ae05 to c5dacd2 Compare October 22, 2024 12:38
@@ -14,11 +14,13 @@

package org.eclipse.ui.internal;


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested 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

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

@fedejeanne
Copy link
Contributor

@jannisCode please adapt the commit message to suit the recommendations

It still looks odd with [df
image

jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 23, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 23, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 23, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
@jannisCode
Copy link
Contributor Author

I now changed the while loop, so that it works better with the example [df. Unfortunately it doesn´t handle very wide characters like m or o as well as before then.
image
image

jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 25, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
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.

As discussed in today's call, here are some tips:

  • Don't pass the ControlDecoration as a parameter, only the Control,
  • Return earlier in case e.getIndex() == -1 and avoid creating a StringBuffer,
  • Extract methods from the whole catch block to improve readability,
  • Use hair spaces (they are thinner than thin spaces)
  • Try to compute the amount of hair spaces that you need to add before the ^. In the end, the ideal result is when the tip of the "^" is placed right in the middle of the offending character.

jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 27, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 28, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
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.

The results are much better now. There are only a few comments about the coding but after they are addressed, this PR will be ready to go 👍

jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 28, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
jannisCode added a commit to jannisCode/eclipse.platform.ui that referenced this pull request Oct 28, 2024
When showing the multi line error message, the arrow ^ now points on the
right character.

eclipse-platform#2407
@fedejeanne
Copy link
Contributor

Failed check: see #2432

@fedejeanne
Copy link
Contributor

@jannisCode which changes did you introduce when you force-pushed 3c3d8cc? I can't see any difference in the code when I compare it to my previous commit (9362717), only that you removed me as co-author.

@jannisCode
Copy link
Contributor Author

jannisCode commented Oct 29, 2024

@fedejeanne I am pretty sure that it was just an update with rebase. I definitely didn´t mean to remove you as co-author

@fedejeanne fedejeanne force-pushed the multi_line_error_message_four branch 2 times, most recently from bf93796 to 4baf0d1 Compare October 29, 2024 10:25
When using a regular expression for a search, if there is enough
information as to where the invalid character of the expression is then
a multiline error message will be shown. The first line contains the
regular expression and the second line contains an arrow (^) that points
to the offending character.
@fedejeanne
Copy link
Contributor

@fedejeanne I am pretty sure that it was just an update with rebase. I definitely didn´t mean to remove you as co-author

👍
I removed some unnecessary empty lines and pushed again. I also wanted to test if the failing tests were unrelated to this PR (I think they are).

After the checks are done, I will merge this. Please do not push any more changes.

@fedejeanne fedejeanne merged commit 2c59dfb into eclipse-platform:master Oct 30, 2024
11 of 14 checks passed
@fedejeanne
Copy link
Contributor

@jannisCode care to add a N&N entry? :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request noteworthy Noteworthy feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants