[RFC][icedtea-web] Fix PR955: regression: SweetHome3D fails to run

[Jiri Vanek]

On 08/01/2012 05:13 PM, Danesh Dadachanji wrote:
> Hello,
>
> Here is a fix for PR955. [1]
>
> This was quite a tricky patch to figure out. Proprietary webstart handles locales.. interestingly.. I've made ours use a fallback mechanism to find any information in the information tag. For each item it searches for (e.g. the title), it will try to find an information tag which has the closest possible locale that looks like your JVM's locale. Please see the unit tests for examples.
>
> I have changed a few other things too with how we setup locales, the variant was not working properly. It was assumed to be 2 characters long which is definitely not the case. My locale is en_CA.utf8 <-- variant has 4 chars.
>
> My changes to the localeMatches method cover both the implementation I needed (being able to match multiple a locale by a given precision - the enum), as well as making it more easily understood. I hope the switch cases do this, it was a bit of a brain teaser figuring out those if statements and double negatives. :/
>
> All other changes are in the ChangeLog below:
> +2012-08-01 Danesh Dadachanji <ddadacha at redhat.com>
> +
> + Fix PR955: regression: SweetHome3D fails to run
> + * NEWS: Added entry for PR955
> + * netx/net/sourceforge/jnlp/JNLPFile.java: New enum Match that represents
> + the level of precision to use when matching locales.
> + (localMatches): Renamed to localeMatches, added matchLevel paramater
> + and updated conditionals to handle the level of precision specified by it.
> + (getVendor): New method that returns an information's vendor text.
> + (getInformation): Added override methods for getTitle and getVendor
> + that are used by the anonymous class to filter by locale. All three
> + methods now go through all levels of precision to search for the best
> + fitted locale.
> + (getResources), (getResourcesDescs): Updated to check if any level of
> + precision matches when searching for locales.
> + (parse): Added call to checkForTitleVendor.
> + * netx/net/sourceforge/jnlp/Parser.java
> + (checkForTitleVendor): New method to check for availability of localized
> + title and vendor from the information tags. Throws ParseException.
> + (getInfo): Replace loop with foreach loop.
> + (getInformationDesc): Remove check for present title and vendor.
> + (getLocale): Variant returned can now use everything after the eigth
> + element of the locale's string.
> + * netx/net/sourceforge/jnlp/resources/Messages.properties:
> + Update missing title and vendor messages to mention localization.
> + * tests/reproducers/simple/InformationTitleVendorParser/testcases/InformationTitleVendorParserTest.java:
> + Update output string as per new changes to Messages internationalizations.
> + * tests/netx/unit/net/sourceforge/jnlp/JNLPFileTest.java:
> + New unit test that checks the localesMatches method in JNLPFile.
> + * tests/netx/unit/net/sourceforge/jnlp/MockJNLPFile.java:
> + New class used to create a mock JNLPFile object.
> + * tests/netx/unit/net/sourceforge/jnlp/ParserTest.java:
> + New unit test that checks that the return of getTitle and getVendor
> + have localized information.
>
> I would like to push this to HEAD and backport it to 1.3. The regression isn't present in 1.2 and before. Any thoughts?
>
> Regards,
> Danesh
> [1] http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=955

Hi! Nice work, I have just few really minor stuff for patch itself and several more for tests.

in JNLPFile there are iner classes ( you have touched them) InformationDesc and ResourcesDesc which are overriding methods but are missing annotations. Can you please add those notations? ( I have counted 5 of them)
You are throwing ParseException  if venodr/title is not found, and later in tests you are testing result against internationalize-able  text. What do you think about more modular  exception chain?
eg ParseException -> RequiredElementException --> (MissingTitleException + MissingVendorException) and then in tests check just for those exceptions without real text?
In Message.properties you are writing
"The title/vendor section has not been defined for your locale in the JNLP file." which looks wrong to me or I understand badly your code. Your code will fallback to the not-localised  if no locale (ab_CD.efg -> ab_CD -> ab -> defualt) or not?
This should be a bit reflecting in error message. eg "The title/vendor section has not been defined for your locale nor is existing its default value in yourJNLP file."
You can also save duplicate entries in properties by something like :

PVendor=vendor
PTitle=title
PNoVendorTitleElement=The {1} section has not been defined for your locale in the JNLP file.

and

throw new SomeBetterThenParseException(R("PNoVendorTitleElement",R("PTitle"/"PVendor")));

but as you wish....



Tests:
MockJNLPFile is in tests directory but is not a test. So test runner will fail on it. This is not what is wanted. To prevent this yo can make this class as inner or move to test-extensions. because this file is already shared I'm for second approach. lets move it eg to test-extensions/net.sourceforge.jnlp.mocks
Unittests will probably also deserve tests with new Locale("X") and new Locale("x","y") (you are now just testing new Locale("x","y","z");
Reproducer test - reproducer is missing:(( I tought that you will include two or three (both failing and passing) with the Locales hack. I have posted repreoducer (without hack!) some time ago, it is not in. Feel free to ruse. But imho reproducer is still worthy.


Thank you very much for both (both excellent!) patch and tests.


J.