[PATCH] Use <different> translation from KeyedItem

[stoecker]

Hmm, your change will make that string untranslated, as there isn't a single place where the string is marked as translatable. Anyway, why is this necessary?

[gaben]

Replying to stoecker:

Hmm, your change will make that string untranslated, as there isn't a single place where the string is marked as translatable.

In KeyedItem.java the DIFFERENT_I18N is already used and translateable to my knowledge.

Anyway, why is this necessary?

Removing the error prone code usage based on previous experience in JOSM.

[stoecker]

Replying to gaben:

Replying to stoecker:

Hmm, your change will make that string untranslated, as there isn't a single place where the string is marked as translatable.

In KeyedItem.java the DIFFERENT_I18N is already used and translateable to my knowledge.

No. As you remove all other places it's not. Software doesn't care how many, but there must be at least one place, where a tr() or trm() is around the string, not a variable of the string. ;-)

[taylor.smock]

@gaben: stoecker is correct in saying that your patch would remove all translations for <different>.

Possible solutions:

1. Wrap the "<different>" string in KeyedItem with marktr
2. Don't apply the patch.

Personally, I probably would have wrapped the "<different>" string with marktr, and used that variable everywhere else (so tr(KeyedItem.DIFFERENT), mostly to make it easier if we ever decide to try and make it possible to change the JOSM language without restarting. But your method has its pro's as well, but mostly if it is called inside a loop on the EDT.

[gaben]

Yep, I know just currently have no time to reduce my JOSM ticket backlog.

Thanks for the heads up. I'll look into it and other tickets as well.

[taylor.smock]

No worries. I'm just going through the tickets marked for 22.10 and applying the ones that are ready/fix a bug and probably won't cause issues (next tested version will either be released this weekend by one of the other maintainers, or early next week by me).

This particular ticket just hasn't shown any movement in a few months, so I dropped the milestone to reduce the number of Milestone: uu.vv -> xx.yy messages.

[gaben]

Suggestion applied, now there is a marktr() around the string. Not sure what's the difference to the simple tr() in this case.

[taylor.smock]

tr does the actual translation, marktr is effectively a no-op, only used to ensure that the translation system doesn't miss strings that are set to variables, but not translated in that context (for whatever reason).

So

static final String DIFFERENT = "different";
System.out.println(tr(DIFFERENT));

would always print "different", since it will never be translated.

static final String DIFFERENT = marktr("different"); // we can now pass the string to tr for the actual translation
System.out.println(tr(DIFFERENT));
// or just
System.out.println(tr("different"));

would work, since the "different" string will be picked up by the translation tools.

[gaben]

Thanks for the explanation! So the second version of the patch is also wrong, as I only used marktr(). I'm going to reupload with this modification.

[gaben]

v2 reupload

[gaben]

Please review and remove the WIP from the title if you find the second version acceptable.

[taylor.smock]

It looks better now.

I would have done things slightly differently, but that is almost a style/implementation detail. Specifically, in source:trunk/src/org/openstreetmap/josm/gui/tagging/presets/items/KeyedItem.java​, I would have used protected static final String DIFFERENT = marktr("<different>"); public static final String DIFFERENT_I18N = tr(DIFFERENT);, but it is arguable as to which style is better.

I'm uncertain about the change to toString in PropertiesDialog -- the String.format code is easier to read, but is a bit slower. I'd have to profile it and see if it slows a code path down too much. I don't think it will, since toString isn't usually called in a loop.

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

In 18692/josm:

Fix #21965: Reuse same instance of "<different>" for consistency (patch by gaben)