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

[Danesh Dadachanji]

On 08/08/12 08:11 AM, Jiri Vanek wrote:
> On 08/07/2012 08:20 PM, Danesh Dadachanji wrote:
>>
>>
>
> I'm ok for head and in some time also for 1.3
>

Awesome, thanks very much for the review! I have one more minor update in the attached patch, I've added printing of homepage and 
description when checking for title/vendor if in debug mode. This will make regression tests a bit easier. =)

Can I get another okay?

Thanks again for all the comments!

Cheers,
Danesh

>> On 02/08/12 09:57 AM, Jiri Vanek wrote:
>>> 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)
>>
>> Ah yes, good catch, done.
>>
>>> 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?
>>
>> Amazing idea, this also makes the unit tests cleaner! I added a third, MissingInformationException
>> is needed as well.
>>
>>> 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?
>>
>> You are correct, I added that change before I fully understood the fallbacks, changing it now.
>>
>>> 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.
>>
>> Neat, I did not know about this. I think it's {0} though for param1.
>
> sorry, sure.
>>
>>>
>>> and
>>>
>>> throw new SomeBetterThenParseException(R("PNoVendorTitleElement",R("PTitle"/"PVendor")));
>>
>> Done.
>>
>>>
>>> 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
>>
>> Great idea, done.
>>
>>> 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");
>>
>> Okay, I duplicated all the tests to use Locale("EN") and to use Locale("EN", "CA"). The tests are
>> the almost identical, I had to swap a few expected failures around. (e.g. Locale("EN", "CA") should
>> fail if the info tag had "en_CA.utf8"). Apart from this, they're pretty much the same tests copy
>> pasted. =)
>>
>>> 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.
>>>
>>>
>>
>> I thought the unit tests + the general info/title/vendor reproducer would be enough so I didn't try
>> to write one earlier. I gave writing one a shot but did not get very far. I'd like to finish off the
>> tests but I just don't have enough cycles left to finish them off. Sorry! Please feel free to take
>> over the rest of the patch.
>>
>> I've attached the latest patch here, hopefully you or someone else can pick it up. Thanks very much
>> for all the reviews!
>>
>> Regards,
>> Danesh
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: title-vendor-required-08.patch
Type: text/x-patch
Size: 99535 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120808/95adc04a/title-vendor-required-08.patch