Request for review on German localizations for icedtea-web

Fri Mar 22 08:19:18 PDT 2013

On 03/20/2013 06:58 AM, Jiri Vanek wrote:
> ping?
>
>
> -------- Original Message --------
> Subject: Re: Request for review on German localizations for icedtea-web
> Date: Fri, 01 Mar 2013 11:59:22 +0100
> From: Jiri Vanek <jvanek at redhat.com>
> To: Jacob Wisor <gitne at excite.co.jp>
> CC: distro-pkg-dev at openjdk.java.net
>
> On 02/28/2013 07:44 PM, Jacob Wisor wrote:
>> Hello Jiri,
>>
>> I have tested your patch. Here are some of my notes regarding it.
> Thanx for check!
>> "Jiri Vanek"<jvanek at redhat.com>  wrote:
>>> On 02/22/2013 03:04 AM, Jacob Wisor wrote:
>>>> Hello there!
>>>>
>>>>>> Jacob, Alexander - in your emails you have come to conclusion, that there
>>>>>> are errors - both lexical
>>>>>> and semantic in original English properties.  If you will have spare time,
>>>>>> do you think - as
>>>>>> separate patch, you can fix them?
>> As I have seen, you have adjusted the relevant message about absolute paths. It is still kind of bothering me because it suggests that there are only two operating systems on this planet. ;) I would rather reformulate it so that it just gives a hint that the specified string is not an absolute path, regardless of its possible form on any given OS. I understand that you want to provide a helpful message to the user but in the world of Java we are probably left with nothing else than just saying that it is not an absolute path, or having specific messages for specific OSs (what I would not advise to do).
> Seams reasonable. All platform specific traces from this line removed.
>
>> Btw, please note that Linux and Windows are names so that the have to begin with a capital letter in English. Further more Windows is a registered trademark and as such there has to be some notion of it in the product, documentation, or has to have an adjacent trademark symbol when mentioned. AFAIK this especially applies to US law but to many other jurisdictions as well. And, for the sake of completeness it should probably be stated as "Microsoft Windows". So as far as that is concerned, I would rather leave out mentioning explicit OS's names.
> Damn :) But I understand....
> If you will be willing to find and mark such a cases it will be really appreciated.  I think you are
> the only person ever who tried to think about our properties in such an point of view.
>> There are even more trademark issues in the Messages.properties file, but I am going to comment on them in my proposals later.
>>
>>>>> *** OK, I can try to have look, but generally sometimes the source is quite
>>>>> confusing and the translator can only guess, what is the correct meaning.
>>>>> Rather the developers themselves should see our comments about unclear
>>>>> parts and rephrase them so that the meaning is clear...
>>>> I can post some proposals for the messages in question. It should not be really that much, but a few of them are utterly confusing indeed.
>>>>
>>>> Btw, as I already wrote to Alexandr, I was able to compile at least NetX on Windows today, so I can see the German and Polish translations in the context of the running application. This helps a lot.
>>>> Having done that, I have also found a minor bug (I did not check whether it has been already reported) in regards to one message on Windows. I have already commented on it. It is the message that refers to absolute paths that must begin with a /. Though absolute paths on Windows do not begin with a / (it is a forbidden character anyway), icedtea-web assumes the (semi-) correct default value. The default value is an absolute Windows path beginning with a drive letter or device. Icedtea-Web still complains about it that it does not begin with a / and prints an error message about the value. It should be easy to fix.
>>>>
>>>> Anyway, I will post an updated localization patch ASAP.
>>>>
>>>> Regards,
>>>> Jacob
>>>>
>>> Wou!
>>> So there will be also Polish trnaslation?
>>> /me amazed :)
>>>
>>> And I'm happy netx is working on windows !o)
>>>
>>> I'm attaching patch to window paths. If you will be able to test, it would be nice (As there is no
>>> win machine around :( ) I will send [rfc] for it as separate email later.
>> So as already mentioned, I have tested it. It compiles and runs. You can have a look at the output log it produces. There are still some cases where the validation does not take effect.
> I see:-/
>
> btw- the IOException your log is ending with will probably be caused by not existing
> C:\Users\Jakob\.icedtea\security\ directory.
> This is definitely bug (as directory should be create din sucha case imho), but ith sould be
> discused in separate thread. (and probably submitted to classpath bugzilla:-/)
>
>> Btw, after reviewing your code I would rather suggest to use java.io.File.isAbsolute(). Please do not try using your own regex pattern to test that, since absolute paths on Windows NT can be even more complex beginning with "\\?\" or "\\.\". There are some further rules to that, so it is probably safest to rely on the work that has already been done by Sun or Oracle. It is even more advisable to use java.io.File.isAbsolute() because every platform specific JRE implementation has their own implementation of it and thus knows best how to check for absoluteness. This way, you do not have to even check for a specific OS before, as you do in boolean net.sourceforge.jnlp.config.BasicValueValidators.canBeWindows(java.lang.String).
> Although I was aware i was not covering all possible win "path beginnings"  I hopped for most of
> them and seeing "/" in code I utterly forgot about isAbsolute method !-( shame in me:((
> So followed, fixed.
>> You do not need to bother about Windows stuff for now - unless you really want to. I am going to build IcedTea-Web for and on Windows anyway. There are even more flaws that should be fixed for Windows. Especially determining the user's and application's data files and folders should be fixed for Windows. So I am going take a look into that.
> Oh tahnk you! It is really loong time since last win build effort. I cross my fingers for you (and
> for us to help you) to finish this issue!
>
> New patch attached.
>
> J.
>
> ps:  considering absolutism of file, as Mac is derived from Linux kernel, I believe from this point
> of view really just two (UNIX like and DOS-like)  exists... (/me hopes O:)
>
>
>
> J.
>
>
>

OK. Doing code review of updated one ... sorry for reviewing the wrong one.

> diff -r 555397961654
> netx/net/sourceforge/jnlp/config/BasicValueValidators.java
> --- a/netx/net/sourceforge/jnlp/config/BasicValueValidators.java Thu
> Feb 28 16:03:39 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/config/BasicValueValidators.java Fri
> Mar 01 11:28:39 2013 +0100
> @@ -37,6 +37,7 @@
>
> package net.sourceforge.jnlp.config;
>
> +import java.io.File;
> import static net.sourceforge.jnlp.runtime.Translator.R;
>
> import java.net.URL;
> @@ -86,8 +87,9 @@
> * Checks if a value is a valid file path (not a valid file!). The actual
> * file may or may not exist
> */
> - private static class FilePathValidator implements ValueValidator {
> -
> + //package private foor testing purposes

s/foor/for/

> + static class FilePathValidator implements ValueValidator {
> +
> @Override
> public void validate(Object value) throws IllegalArgumentException {
> if (value == null) {
> @@ -97,13 +99,15 @@
> Object possibleValue = value;
>
> if (!(possibleValue instanceof String)) {
> - throw new IllegalArgumentException();
> + throw new IllegalArgumentException("Value should be string!");
> }
>
> String possibleFile = (String) possibleValue;
> - if (!(possibleFile.startsWith("/"))) {
> - throw new IllegalArgumentException();
> - }
> +
> + boolean absolute = new File(possibleFile).isAbsolute();
> + if (!absolute) {
> + throw new IllegalArgumentException("File must be absolute");
> + }
>
> }
>
> diff -r 555397961654
> netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Thu Feb
> 28 16:03:39 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Fri Mar
> 01 11:28:39 2013 +0100
> @@ -283,7 +283,7 @@
> # Value Validator messages. Messages should follow "Possible values ..."
> VVPossibleValues=Possible values {0}
> VVPossibleBooleanValues=are {0} or {1}
> -VVPossibleFileValues=include the absolute location of a file - it
> must begin with a /
> +VVPossibleFileValues=include the absolute location of a file
> VVPossibleRangedIntegerValues=are in range {0} to {1} (inclusive)
> VVPossibleUrlValues=include any valid url (eg
> http://icedtea.classpath.org/hg/)
>
> diff -r 555397961654
> tests/netx/unit/net/sourceforge/jnlp/config/BasicValueValidatorsTests.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++
> b/tests/netx/unit/net/sourceforge/jnlp/config/BasicValueValidatorsTests.java
> Fri Mar 01 11:28:39 2013 +0100
> @@ -0,0 +1,136 @@
> +/* DeploymentConfigurationTest.java
> +Copyright (C) 2012 Red Hat, Inc.

s/2012/2013/

> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as
> published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +package net.sourceforge.jnlp.config;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +public class BasicValueValidatorsTests {
> +
> + //decomposed for testing
> + public static boolean canBeWindows(String s) {
> + //is just win bullet proof enough? SOme winfows mark themselves as
> win32 or similar
> + //but I can imagine linux distribution with some Name containing win:(
> + //so I guess windows should be good enough as I have not seen
> not-windows idf since XP
> + return s.toLowerCase().contains("windows");

s/SOme/some/, s/winfows/Windows/, s/Name/name/
idf -> ???

Anyway, I haven't see any reference that os.name can return 'win32', do
you have one ?
[nit] I'd be in favour of a shorter note here, it's a bit of a ramble :-)

> + }
> +
> + /**
> + * guess the OS of user, if legal, or windows
> + * @return
> + */
> + public static boolean canOsBeWindows() {
> + return canBeWindows(System.getProperty("os.name"));
> + }

>From what I read on the internet,
System.getProperty("os.name").startsWith("Windows") is the accepted way
to do this. (I don't think we need to worry about this being inadequate
until we have proof it doesn't handle some version of windows).
[nit] I'd just call it isWindows() or isOsWindows() or similar.

> + private static final BasicValueValidators.FilePathValidator pv = new
> BasicValueValidators.FilePathValidator();
> + private final String neverLegal = "aaa/bb/cc";
> + private final String winLegal = "C:\\aaa\\bb\\cc";
> + private final String linuxLegal = "/aaa/bb/cc";
> +
> + @Test
> + public void testWindowsDetction() {
> + Assert.assertTrue(canBeWindows("blah windows blah"));
> + Assert.assertTrue(canBeWindows("blah Windows blah"));
> + Assert.assertTrue(canBeWindows(" WINDOWS 7"));
> + Assert.assertFalse(canBeWindows("blah windy miracle blah"));
> + Assert.assertFalse(canBeWindows("blah wind blah"));
> + Assert.assertTrue(canBeWindows("windows"));
> + Assert.assertFalse(canBeWindows("linux"));
> + Assert.assertFalse(canBeWindows("blah mac blah"));
> + Assert.assertFalse(canBeWindows("blah solaris blah"));
> + }
> +
> + @Test
> + public void testLinuxAbsoluteFilePathValidator() {
> + if (!canOsBeWindows()) {
> + Exception ex = null;
> + try {
> + pv.validate(linuxLegal);
> + } catch (Exception eex) {
> + ex = eex;
> + }
> + Assert.assertTrue(ex == null);
> +
> + ex = null;
> + try {
> + pv.validate(neverLegal);
> + } catch (Exception eex) {
> + ex = eex;
> + }
> + Assert.assertTrue(ex instanceof IllegalArgumentException);
> +
> +
> + ex = null;
> + try {
> + pv.validate(winLegal);
> + } catch (Exception eex) {
> + ex = eex;
> + }
> + Assert.assertTrue(ex instanceof IllegalArgumentException);
> + }
> + }
> +
> + @Test
> + public void testWindowsAbsoluteFilePathValidator() {
> + if (canOsBeWindows()) {
> + Exception ex = null;
> + try {
> + pv.validate(winLegal);
> + } catch (Exception eex) {
> + ex = eex;
> + }
> + Assert.assertTrue(ex == null);
> +
> + ex = null;
> + try {
> + pv.validate(neverLegal);
> + } catch (Exception eex) {
> + ex = eex;
> + }
> + Assert.assertTrue(ex instanceof IllegalArgumentException);
> +
> +
> + ex = null;
> + try {
> + pv.validate(linuxLegal);
> + } catch (Exception eex) {
> + ex = eex;
> + }
> + Assert.assertTrue(ex instanceof IllegalArgumentException);
> + }
> + }
> +}

Looks good otherwise, I think you can make changes to test without
posting again.
-Adam