Opened 11 years ago
Closed 11 years ago
#4339 closed enhancement (fixed)
MessageFormat fixes
Reported by: | 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)
Change History (19)
Changed 11 years ago by
Attachment: | messageformat-fixes.diff added |
---|
Changed 11 years ago by
Attachment: | more-messageformat-fixes.diff added |
---|
comment:1 Changed 11 years ago by
Instead of changing the ' to , please change as follows:
- Wont has no --> wont
- can't --> cannot
- isn't --> is not
- ...
comment:2 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
comment:5 Changed 11 years ago by
I've applied the second patch, removing escaped single quotes in these cases looks fine.
comment:6 Changed 11 years ago by
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 11 years ago by
Shouldn't we rather unify the different tr*() implementations to always treat strings the same?
comment:8 Changed 11 years ago by
There are also a number of cases where tr() is called with variable text. Doesn't that create problems, too?
comment:9 Changed 11 years ago by
Also, there are some calls like tr(""). What is the purpose of that?
comment:10 follow-up: 11 Changed 11 years ago by
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 Changed 11 years ago by
Summary: | [PATCH] MessageFormat fixes → MessageFormat 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 becometr("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 oftr("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 nexttested
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 11 years ago by
I already have a script for the change of english strings. Nothing for translated strings thought.
comment:13 follow-up: 15 Changed 11 years ago by
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:15 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
These issues should be solved.
Some more fixes, where I'm not sure, if they are necessary.