Modify

Opened 10 years ago

Closed 10 years ago

#4339 closed enhancement (fixed)

MessageFormat fixes

Reported by: andre68@… Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description

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.

Attachments (2)

messageformat-fixes.diff (13.7 KB) - added by andre68@… 10 years ago.
more-messageformat-fixes.diff (6.7 KB) - added by andre68@… 10 years ago.
Some more fixes, where I'm not sure, if they are necessary.

Download all attachments as: .zip

Change History (19)

Changed 10 years ago by andre68@…

Attachment: messageformat-fixes.diff added

Changed 10 years ago by andre68@…

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

comment:1 Changed 10 years ago by stoecker

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

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

comment:2 Changed 10 years ago by 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.

comment:3 Changed 10 years ago by 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 :-)

comment:4 Changed 10 years ago by Gubaer

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

comment:5 Changed 10 years ago by Gubaer

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

comment:6 Changed 10 years ago by 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.

comment:7 Changed 10 years ago by mjulius

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

comment:8 Changed 10 years ago by mjulius

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

comment:9 Changed 10 years ago by mjulius

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

comment:10 Changed 10 years ago by 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.

comment:11 in reply to:  10 Changed 10 years ago by mjulius

Summary: [PATCH] MessageFormat fixesMessageFormat fixes

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.

comment:12 Changed 10 years ago by stoecker

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

comment:13 Changed 10 years ago by 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.

comment:14 Changed 10 years ago by Gubaer

The link is CheckParameterUtil

comment:15 in reply to:  13 Changed 10 years ago by 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));

comment:16 Changed 10 years ago by 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.

comment:17 Changed 10 years ago by mjulius

Resolution: fixed
Status: newclosed

These issues should be solved.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.