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

extension mechanism for ignore white space action of the compare view #429

Merged
merged 1 commit into from
May 4, 2023
Merged

extension mechanism for ignore white space action of the compare view #429

merged 1 commit into from
May 4, 2023

Conversation

tobiasmelcher
Copy link
Contributor

The "Ignore White Space" context menu action in the eclipse compare view is ignoring all whitespaces in the source code, including the significant ones. Whitespace differences in string literals in programming languages like Java or ABAP for example are quite important and should not simply be ignored. We got at least this feedback from our users of the ABAP Development Tools here at SAP.

The changes in this pull request allow tool implementers to hook into the ignore whitespace algorithm by providing an implementation of interface IIgnoreWhitespaceContributor; similar to TextMergeViewer#createTokenComparator, a new method TextMergeViewer#createIgnoreWhitespaceContributor is introduced which can be overriden when creating the TextMergeViewer in the IViewerCreator#createViewer extension.

IIgnoreWhitespaceContributor#isIgnoredWhitespace is called in the "Ignore White Space" action run for each found whitespace and the implementer can decide whether the whitespace can be really ignored or not if it is part of a string literal.

The "Text Compare" IViewerCreator implementation will ignore all whitespaces because it cannot have any knowledge about the programming language. Language specific IViewerCreator implementations like "Java Compare" or "ABAP Compare" can now influence the ignore whitespace behavior and include all programming language relevant whitespaces inside string literals or elsewhere.

I worked on this pull request as part of the "Contributing to Eclipse" Hackathon here at SAP and it would be great if you could take a look and review it? Feedback is very welcome.

@BeckerWdf
Copy link
Contributor

@tobiasmelcher Thanks for the contribution.

BeckerWdf
BeckerWdf reviewed Apr 28, 2023
View reviewed changes
Copy link
Contributor

@BeckerWdf BeckerWdf left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Apr 28, 2023 •
edited

Test Results

     30 files  ±0       30 suites  ±0   47m 5s ⏱️ + 4m 37s
2 380 tests +2  2 379 ✔️ +2  1 💤 ±0  0 ❌ ±0 
7 143 runs  +6  7 140 ✔️ +6  3 💤 ±0  0 ❌ ±0 

Results for commit 00a41e5. ± Comparison against base commit ddde72d.

♻️ This comment has been updated with latest results.

laeubi
laeubi approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

LGTM

@BeckerWdf
Copy link
Contributor

@tobiasmelcher
Can you add automated tests for this feature?

iloveeclipse
iloveeclipse requested changes May 2, 2023
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

Please

  1. Squash all commits to one.
  2. Add tests for new API.

@BeckerWdf
Copy link
Contributor

  1. Squash all commits to one.

This can be done at the point-in-time when merging the PR.

@BeckerWdf
Copy link
Contributor

Add tests for new API.

Yes please.

@iloveeclipse
Copy link
Member

This can be done at the point-in-time when merging the PR.

Yes, it could, but there will be 4 commit messages to re-write by committer. Why not provide a proper commit with proper commit message written by contributor?

@BeckerWdf
Copy link
Contributor

This can be done at the point-in-time when merging the PR.

Yes, it could, but there will be 4 commit messages to re-write by committer. Why not provide a proper commit with proper commit message written by contributor?

I think that multiple commits make it easier to follow the code review flow. A force push after a first code review makes it impossible to follow the code review comment - or I did not find the possibility.

iloveeclipse
iloveeclipse reviewed May 2, 2023
View reviewed changes
team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/DocLineComparator.java Outdated
private static boolean isSignificantWhitespace(String line, int columnNumber, char character, int lineNumber,
DocLineComparator comparator) {
boolean isWhitespace = Character.isWhitespace(character);
if (isWhitespace && comparator.fIgnoreWhitespaceContributor != null
Copy link
Member

Choose a reason for hiding this comment

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

May I suggest to avoid == false comparision and instead something like

		IIgnoreWhitespaceContributor ignoreWhitespaceContributor = comparator.fIgnoreWhitespaceContributor;
		if (isWhitespace && ignoreWhitespaceContributor != null
				&& !ignoreWhitespaceContributor.isIgnoredWhitespace(lineNumber, columnNumber)) {
			isWhitespace = false; // whitespace should be included and not ignored
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please take a look at 9f6f105

Copy link
Contributor

Choose a reason for hiding this comment

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

please take a look at 9f6f105

@iloveeclipse: Can you review again? Are you fine with Tobias change?

@tobiasmelcher
Copy link
Contributor Author

[c16b8f2](/eclipse-platform/eclipse.platform/pull/429/commits/c16b8f23294b10b8bab820eedde70803df0dc9ba)

unit tests are implemented with c16b8f2. Could you please review? Thanks.

BeckerWdf
BeckerWdf requested changes May 3, 2023
View reviewed changes
Copy link
Contributor

@BeckerWdf BeckerWdf left a comment

Choose a reason for hiding this comment

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

See my change proposals

iloveeclipse
iloveeclipse requested changes May 3, 2023
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

Please also squash all commits to one and provide one commit with a clear commit message and description what APi was added etc.

...dles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java Show resolved Hide resolved
@tobias-melcher
extension mechanism for ignore whitespace action of the compare view
00a41e5 
The "Ignore White Space" context menu action in the eclipse compare view
is ignoring all whitespaces in the source code, including the
significant ones. Whitespace differences in string literals in
programming languages like Java for example are quite important and
should not simply be ignored.

Allow tool implementers to hook into the ignore whitespace algorithm by
providing an implementation of interface IIgnoreWhitespaceContributor;
similar to TextMergeViewer#createTokenComparator, a new method
TextMergeViewer#createIgnoreWhitespaceContributor is introduced which
can be overridden when creating the TextMergeViewer in the
IViewerCreator#createViewer extension.
@BeckerWdf BeckerWdf added this to the 4.28 M3 milestone May 4, 2023
@BeckerWdf BeckerWdf merged commit 4e8e3bf into eclipse-platform:master May 4, 2023
14 checks passed
@BeckerWdf
Copy link
Contributor

Thanks @tobiasmelcher for this nice addition.

@BeckerWdf
Copy link
Contributor

Do we need a new and noteworthy for this API addition?

@BeckerWdf BeckerWdf mentioned this pull request May 4, 2023
Open
@iloveeclipse
Copy link
Member

Do we need a new and noteworthy for this API addition?

Would be nice

@BeckerWdf
Copy link
Contributor

Do we need a new and noteworthy for this API addition?

Would be nice

How do I do that? If you give me a hint I ill provide some.

@merks
Copy link
Contributor

merks commented May 9, 2023

Here's a recent example that I did:

eclipse-platform/www.eclipse.org-eclipse-news#95

BeckerWdf added a commit to BeckerWdf/www.eclipse.org-eclipse-news that referenced this pull request May 9, 2023
@BeckerWdf
N&N Entry for IgnoreWhitespace Contribution
868e1e9 
See: eclipse-platform/eclipse.platform#429
@BeckerWdf BeckerWdf mentioned this pull request May 9, 2023
Merged
BeckerWdf added a commit to eclipse-platform/www.eclipse.org-eclipse-news that referenced this pull request May 9, 2023
@BeckerWdf
N&N Entry for IgnoreWhitespace Contribution (#96)
a108729 
See: eclipse-platform/eclipse.platform#429
@BeckerWdf
Copy link
Contributor

Here's a recent example that I did:

eclipse-platform/www.eclipse.org-eclipse-news#95

Thanks. I added a N&N entry (see eclipse-platform/www.eclipse.org-eclipse-news#96)

merks added a commit to merks/www.eclipse.org-eclipse-news that referenced this pull request May 9, 2023
@merks
Improve the N&N Entry for IgnoreWhitespace Contribution
b7d1c3b 
See: eclipse-platform/eclipse.platform#429
@merks merks mentioned this pull request May 9, 2023
Merged
BeckerWdf pushed a commit to eclipse-platform/www.eclipse.org-eclipse-news that referenced this pull request May 9, 2023
@merks
Improve the N&N Entry for IgnoreWhitespace Contribution (#97)
1b62696 
See: eclipse-platform/eclipse.platform#429
@BeckerWdf BeckerWdf mentioned this pull request May 17, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laeubi laeubi laeubi approved these changes

@ruspl-afed ruspl-afed ruspl-afed left review comments

@iloveeclipse iloveeclipse iloveeclipse approved these changes

@BeckerWdf BeckerWdf BeckerWdf approved these changes

Assignees

@tobiasmelcher tobiasmelcher

Labels
None yet
Projects
None yet
Milestone
4.28 M3
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tobiasmelcher @BeckerWdf @iloveeclipse @merks @laeubi @ruspl-afed @tobias-melcher