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

Danesh Dadachanji ddadacha at redhat.com
Wed Aug 8 08:51:24 PDT 2012



On 08/08/12 11:27 AM, Jiri Vanek wrote:
> On 08/08/2012 04:50 PM, Danesh Dadachanji wrote:
>> 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?
>
> yy, go on.

Thanks! Pushed here:
http://icedtea.classpath.org/hg/icedtea-web/rev/1d033579d40e

>>
>> 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
>>>
>



More information about the distro-pkg-dev mailing list