[icedtea-web] Regression after refactoring for XDG specification

[Jacob Wisor]

Jiri Vanek schrieb:
> On 07/30/2013 06:14 PM, Jacob Wisor wrote:
>> "Jiri Vanek"<jvanek at xxxxxxxxxx> wrote:
>> […]
>> Looks good. Just one other thing:
>>
>> + File f = new File(value);
>> + if (f.exists()) {
>> + if (JNLPRuntime.isDebug()) {
>> + System.out.println("OK: key " + key + " value: " + value + " exists");
>> + }
>> + continue;
>> + }
>> + if (!f.mkdirs()) {
>> + if (JNLPRuntime.isDebug()) {
>> + System.out.println("ERROR: key " + key + " value: " + value + " not existed, and was NOT created");
>>
>> This text should probably be written to stderr.
>>
>> + }
>> + } else {
>> + if (JNLPRuntime.isDebug()) {
>> + System.out.println("OK: key " + key + " value: " + value + " not existed, and was created");
>> + }
>> + }
>>
>> If the folder is a file that already exists, the folder in which the folder is created is write protected or the current user has no permission to write to it, or the path is for some reason not available (e.g. failed network connectivity) this circumstance is not handled gracefully and the user is not notified that his/her configuration cannot be saved. All that a user will get is a possibly cryptic exception on stderr that some value or property cannot be saved when clicking OK. The exact same exception pops up when the .cache folder could not have been silently created and then the cache viewer is opend. This is similar to the backup problem.
>>
> 
> Ok.Here it is. I think it is working pretty fine. The only thing I'm afraid is behavior of some reproducers. But with some luck this check will remain silent (== passed) and so no dialogue will be raised - not even under most terrific testcases;)
> 
>> This may seem as nit picking, but it actually adds greatly to the usability of the application in case of an error or unusual system configuration.
>>
>> Nevertheless, thank you for addressing this issue, so please keep up the good work.

> +    private static void ensureMainDirs() {

I am especially unhappy with this method's naming. I am aware that naming is a
difficult task in CS, but it pays off investing time into it. What does it
ensure main directories about? What happens if something cannot be or has not
been ensured? Does possibly a RuntimeException be thrown? Except from that, what
are main directories supposed to be? Perhaps root directories? What makes a
directory so distinct to be "main"? So, please find an adequate name and add
some documentation.

Some language stuff:
> +                    System.out.println("WARNING: key " + key + " do not have 
value, switching to default");

"WARNING: key " + key + " has no value, setting to default value"

> +                    System.out.println("WARNING: key " + key + " do not have 
value, skipping");

"WARNING: key " + key + " has no value, skipping"

> +                    System.err.println("ERROR: key " + key + " value: " + 
value + " not existed, and was NOT created");

"ERROR: Directory " + value + " denoted by key " + key + " does not exist and
has not been created"

> +                    System.out.println("OK: key " + key + " value: " + value 
+ " not existed, and was created");

"OK: Directory " + value + " denoted by key " + key + " did not exist but has
been created"

> +    //not private for testing
> +    static String testMainDir(File f, boolean verbose) {

Since you have made this method not private please add some documentation on
what this method's purpose is and what it does. It is definitely not
self-explanatory, even after reading its code. And, as mentioned below it is
probably unnecessary complex and seems to be mixing functional code with message
generating code. If you really want to keep this differentiated error reporting
feature, I would strongly advise to deglomerate functional and message
generating code.

> +        StringBuilder messages = new StringBuilder();
> +        if (!f.exists()) {
> +            String s = (R("DCmaindircheckNotexists", f.getAbsolutePath()));
> +            if (verbose) {
> +                System.err.println(s);
> +            }
> +            messages.append(s).append("\n");
> +        }
> +        if (!f.isDirectory()) {
> +            String s = (R("DCmaindircheckNotdir", f.getAbsolutePath()));
> +            if (verbose) {
> +                System.err.println(s);
> +            }
> +            messages.append(s).append("\n");
> +        }

These two tests are redundant. File.isDirectory() implies a check for existence.
It does not matter to the user or the application whether the desired directory
does not exist or is not a directory. In effect the program cannot read
from/write to the desired location.

> +        File testFile = null;
> +        boolean ok = false;

Naming again. What is ok?

> +        try {
> +            testFile = File.createTempFile("maindir", "check", f);
> +            if (testFile.exists()) {
> +                ok = true;
> +            }
> +            try {
> +                FileUtils.saveFile("ww", testFile);
> +                String s = FileUtils.loadFileAsString(testFile);
> +                if (!s.trim().equals("ww")) {
> +                    ok = false;
> +                }

This test misses probably its point because it is testing for creating a *file*
not a *directory*. Most file systems with DACLs distinguish between /creating/
files or directories, and between /creating/ and /writing/ into files or
directories, so it is important to be precise here. Furthermore, it tests
writing into a file instead of perhaps writing into a file in a subdirectory.

> +            } catch (Exception ex) {
> +                if (JNLPRuntime.isDebug()) {
> +                    ex.printStackTrace();
> +                }
> +                ok = false;
> +            }

No really, you should *not* catch arbitrary exceptions. It ain't just some empty
words.

After all, this method is overly complex and does not add any value for the
user. So I would advise you to discard this method.

> diff -r 79bdc074df81 netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties	Fri Aug 30 
11:02:08 2013 -0400
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties	Tue Sep 03 
13:56:53 2013 +0200
> @@ -304,6 +304,9 @@
>  DCInternal=Internal error: {0}
>  DCSourceInternal=<internal>
>  DCUnknownSettingWithName=Property "{0}" is unknown.
> +DCmaindircheckNotexists=After all tryes your configuration directory {0} do 
not exists
> +DCmaindircheckNotdir=Your configuration directory {0} is not directory
> +DCmaindircheckRwproblem=Your configuration directory {0} can not be 
read/written properly

Although it is nice of you to make precise distinctions between error types to
help the user it is actually not necessary and does not offer any additional
help when analyzing this problem. All these messages can be fused into one
sufficiently explanatory error message: DCCannotAccessConfig="Cannot access
configuration in file {full path to config file}." This should probably be
enough for an admin, even for a user. I would really advise this because it
makes the code simpler and yet serves its purpose.


Regards,
Jacob