MessageFormat fixes

[andre68@…]

The attached patch fixes some MessageFormat texts which (according to JOSM-Dev mailing list) are faulty in the current state.

According to launchpad, there are two more messages in plugins:
plugins/wmsplugin/src/wmsplugin/WMSGrabber.java:115
plugins/openstreetbugs/src/org/openstreetmap/josm/plugins/osb/api/NewAction.java:76

Please check this and apply the patch if necessary.

[andre68@…]

Some more fixes, where I'm not sure, if they are necessary.

[stoecker]

Instead of changing the ' to , please change as follows:

• Wont has no --> wont
• can't --> cannot
• isn't --> is not
• ...

[andre68@…]

Ok, if this is really what it should be, than I will do it... but not today.

Did you look at the second patch? It is removing some meta-characters.

[stoecker]

The "Can't" should be cannot. The others I would leave as they are. " is as good as ' when using as quote and I would rather prevent changing existing strings in this case.

It seems wiki format stripped my quotes :-)

[Gubaer]

(In [2822]) see #4339: applied patch by andre68: MessageFormat fixes

[Gubaer]

I've applied the second patch, removing escaped single quotes in these cases looks fine.

[andre68@…]

Ok, I will work on the other patch after the next stable release, since I'm busy for the next days and this would influence a lot of texts for translation on Launchpad.

[mjulius]

Shouldn't we rather unify the different tr*() implementations to always treat strings the same?

[mjulius]

There are also a number of cases where tr() is called with variable text. Doesn't that create problems, too?

[mjulius]

Also, there are some calls like tr(""). What is the purpose of that?

[Gubaer]

There are also a number of cases where tr() is called with variable text. Doesn't that
create problems, too?

In most cases this should be fixed. Do you mean something like tr("Got object " + o.getId())? This should indeed become tr("Got object {0}", o.getId()).

Also, there are some calls like tr(""). What is the purpose of that?

Absolutely. tr() will replace the empty string with a large, strangely looking text. I don't know where the text comes from but it looks like some kind of manifest. We already run into that when empty error messages were sent through tr().

Shouldn't we rather unify the different tr*() implementations to always treat strings the same?

I think so. I'd change the behaviour of tr("xyz") to reflect the behaviour of tr("xyz", v1,v2,...)}. This will have quite an impact, though. All localized strings including single quotes would have to be updated to reflect the new rule that single quotes always have to be escaped. And not only the localized strings, but also all translations in all supported languages. We might be short of time if we stick with the current schedule to release the next tested end of january.

[mjulius]

OK, I am working on this.

Replying to Gubaer:

There are also a number of cases where tr() is called with variable text. Doesn't that
create problems, too?

In most cases this should be fixed. Do you mean something like tr("Got object " + o.getId())? This should indeed become tr("Got object {0}", o.getId()).

Usually it is something like tr(message).

Also, there are some calls like tr(""). What is the purpose of that?

Absolutely. tr() will replace the empty string with a large, strangely looking text. I don't know where the text comes from but it looks like some kind of manifest. We already run into that when empty error messages were sent through tr().

I just wonder how this got in there in the first place and what the purpose of it is.

Shouldn't we rather unify the different tr*() implementations to always treat strings the same?

I think so. I'd change the behaviour of tr("xyz") to reflect the behaviour of tr("xyz", v1,v2,...)}. This will have quite an impact, though. All localized strings including single quotes would have to be updated to reflect the new rule that single quotes always have to be escaped. And not only the localized strings, but also all translations in all supported languages. We might be short of time if we stick with the current schedule to release the next tested end of january.

Personally, I would vote for letting the schedule slip by a week or so and just get things right. Since it will be the last Java 1.5 release I would put a little more effort into that. We just need to do it now and then give translators enough time to catch up.

Anyway, most of these changes don't require a real translator to fix. They could even be automated. Also, changing all "don't"s and "won't"s are trivial to fix in translations because they don't change the translations at all.

Also, I am planning on getting rid of a number of localized exception messages like IllegalArgumentException(tr("Parameter ''{0}'' must not be null.", "layer")) (Shouldn't that be a NPE anyway?). This message does not tell the user anything.

[stoecker]

I already have a script for the change of english strings. Nothing for translated strings thought.

[Gubaer]

I just wonder how this got in there in the first place and what the purpose of it is.

Me too. But probably it will turn out that I'm to blame for a couple of them too ;-)

Also, I am planning on getting rid of a number of localized exception messages like
IllegalArgumentException(tr("Parameter {0} must not be null.", "layer")) (Shouldn't that be
a NPE anyway?). This message does not tell the user anything.

I'd say it would be an NPE if you actually run into a null pointer. assert or throwing an AssertException would be another alternative. assert shouldn't be used because it can be switched on and off.

BTW: there is a static utility function ensureParameterNotNull in CheckParameterUtil. The many tr("Parameter {0} ....") should be replaced by a call to this method.

[Gubaer]

The link is CheckParameterUtil​

[mjulius]

Replying to Gubaer:

I'd say it would be an NPE if you actually run into a null pointer.

Isn't that the case here?

        if (value == null)
            throw new IllegalArgumentException(tr("Parameter ''{0}'' must not be null", parameterName));

[Gubaer]

Isn't that the case here?

I have no conclusive arguments, I think it's merely a question of "style". If I'd check for a null pointer somewhere in a method and if I got the respective reference with the unexpected null value from somewhere else, for instance as return from from a Swing API call, I'd probably throw an NPE too. However, for any violated method precondition concerning parameters, including unexpected null values, I always throw an IllegalArgumentException. For violated method preconditions concering the state of the target object I tend to throw IllegalStateExceptions.

This also leads to standardised documentation of the method contract: for every checked precondition you have an @throws IllegalArgumentException reason.

[mjulius]

These issues should be solved.