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

Externalize the 'difference/s' text #1236 #1248

Conversation

lathapatil
Copy link
Contributor

Externalization of text used in "Textmergeviewer.java" related to number of differences shown in toolbar of compare editor.

Fixes #1236

Copy link
Contributor

github-actions bot commented Mar 13, 2024

Test Results

   636 files  ±0     636 suites  ±0   43m 40s ⏱️ +38s
 3 945 tests ±0   3 922 ✅ ±0   23 💤 ±0  0 ❌ ±0 
12 441 runs  ±0  12 277 ✅ ±0  164 💤 ±0  0 ❌ ±0 

Results for commit b69f054. ± Comparison against base commit 9d8424b.

♻️ This comment has been updated with latest results.

String label = differenceCount > 1 ? differenceCount + " Differences" //$NON-NLS-1$
: differenceCount == 1 ? differenceCount + " Difference" : "No Difference"; //$NON-NLS-1$ //$NON-NLS-2$
String label = differenceCount > 1 ? differenceCount + CompareMessages.TextMergeViewer_differences
: differenceCount == 1 ? differenceCount + CompareMessages.TextMergeViewer_difference : CompareMessages.TextMergeViewer_no_difference; //$NON-NLS-1$ //$NON-NLS-2$
Copy link
Member

Choose a reason for hiding this comment

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

This is still not good for localization. There are languages in which the plural for two items is different from the plural of > 2 items. Just take a look at https://lingohub.com/blog/pluralization#cldr-overview . Pluralization is hard.

A better way to do this is to leave the choice to the translator. Using MessageFormat.format you could define a single message

TextMergeViewer_differences={0,choice,0#No differences|1#1 difference|1<{0} differences}

and then

String label = MessageFormat.format(CompareMessages.TextMergeViewer_differences, differenceCount);

This gives translators much more leeway in producing correct translations. It'll also format the number according to localized rules, for instance with thousands separators.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know either that this was possible. Nice
@lathapatil: Can you adapt your PR accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will check on this.

Copy link
Member

Choose a reason for hiding this comment

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

It is possible, though standard Java MessageFormat still is not powerful enough to handle all cases. A translator would still struggle to get the right plurals for most slavic languages. ICU has full support for doing this right, but unfortunately Eclipse ditched ICU a while ago "because it was too big".

Copy link
Contributor

@vogella vogella Mar 15, 2024

Choose a reason for hiding this comment

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

The main reason why we ditched ICU was that maintenance of this library was not actively done. IIRC there was one part time person working on it in IBM with his focus on other things.

@lathapatil lathapatil force-pushed the Issues/1236_compareEditor_text_externalization branch 2 times, most recently from c87e639 to 66c2f4d Compare March 18, 2024 07:01
@BeckerWdf
Copy link
Contributor

"Expected to have bigger x.y.z than what is available in baseline (3.10.0.v20240208-0728)"

You need to increase the version number of org.eclipse.compare to 3.10.100

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.

increase version number

@lathapatil
Copy link
Contributor Author

increase version number

ok. I have one basic question regarding version bump . When and why we have to increase the version now for this fix ?

@BeckerWdf
Copy link
Contributor

increase version number

ok. I have one basic question regarding version bump . When and why we have to increase the version now for this fix ?

The version number has to be increase once each release cycle if changes are made to the code in the bundle.
If you only change the implementation but the API stays unchanged you only change the number after the 2nd dot.
If you change API (e.g. adding a new API method) you change the number after the 1st dot.
If you change API incompatible (e.g. removing API) you change the number in front of the 1st dot.

@lathapatil lathapatil force-pushed the Issues/1236_compareEditor_text_externalization branch 2 times, most recently from 9c89b82 to b726c79 Compare March 19, 2024 08:16
@BeckerWdf
Copy link
Contributor

AllCompareTests org.eclipse.compare.tests.TextMergeViewerTest is failing. Might this be caused by your change?

@lathapatil
Copy link
Contributor Author

lathapatil commented Mar 20, 2024

AllCompareTests org.eclipse.compare.tests.TextMergeViewerTest is failing. Might this be caused by your change?

Yes. Previously it was Uppercase 'D' in the text "Differences" and now It is lowercase 'd'

Either I have to change in testcase or the properties file .. which one is preferred ?

@BeckerWdf
Copy link
Contributor

AllCompareTests org.eclipse.compare.tests.TextMergeViewerTest is failing. Might this be caused by your change?

Yes. Previously it was Uppercase 'D' in the text "Differences" and now It is lowercase 'd'

Either I have to change in testcase or the properties file .. which one is preferred ?

A it's a "title" / "caption" this should be title-case style.

Externalization of text used in "Textmergeviewer.java" related to number
of differences shown in toolbar of compare editor using
MessageFormat.format
Version bump to 3.10.100
Update team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareMessages.properties

Co-authored-by: Matthias Becker <ma.becker@sap.com>
@lathapatil lathapatil force-pushed the Issues/1236_compareEditor_text_externalization branch from 304b833 to b69f054 Compare March 20, 2024 09:06
@BeckerWdf BeckerWdf merged commit 072423e into eclipse-platform:master Mar 25, 2024
13 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Texts not externalised
4 participants