[rfc][icedtea-web] fixinf of desktop icon behaviour

[Jiri Vanek]

On 01/02/2013 06:14 PM, Jiri Vanek wrote:
> On 12/28/2012 06:15 PM, Adam Domurad wrote:
>> On 12/24/2012 03:51 PM, Jiri Vanek wrote:
>>> On 12/24/2012 03:06 PM, Jiri Vanek wrote:
>>>> On 12/21/2012 07:55 PM, Jiri Vanek wrote:
>>>>> On 12/21/2012 06:04 PM, Xerxes Rånby wrote:
>>>>>> 2012-12-21 17:05, Jiri Vanek skrev:
>>>>>>> Hi! This is fix for http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=725 - JNLP
>>>>>>> applications will prompt for creating desktop shortcuts every time they are run
>>>>>>>
>>>>>>> When I have seen Omairs original patch it was too much complicated, So I want at least to try
>>>>>>> this one.
>>>>>>> It is pretty simple. The disadvantage however is:
>>>>>>> + public File getFinalLinuxDesktopIconFile() {
>>>>>>> + return new
>>>>>>> File(System.getProperty("user.home")+"/Desktop/"+getDesktopIconFinalName()+".desktop");
>>>>>>> + }
>>>>>>>
>>>>>>> to be dependent on System.getProperty("user.home")+"/Desktop/" and .desktop suffix.
>>>>>>>
>>>>>>> However .. can it be enough?
>>>>>>>
>>>>>>> The second patch is for testing purposes of this case which I would like to push forward as
>>>>>>> soon as possible.
>>>>>>>
>>>>>>> J.
>>>>>>
>>>>>> This will be broken on non-english systems.
>>>>>>
>>>>>> The name of the Desktop folder gets localized on many distributions.
>>>>>> For example the Desktop folder is called Skrivbord on Swedish user systems.
>>>>>> The configuration of the user desktop folder location is set in the
>>>>>> ~/.config/user-dirs.dirs
>>>>>>
>>>>>> On my system this file contains:
>>>>>> xranby at xranby-ESPRIMO-P7935:~/.config$ cat user-dirs.dirs
>>>>>> # This file is written by xdg-user-dirs-update
>>>>>> # If you want to change or add directories, just edit the line you're
>>>>>> # interested in. All local changes will be retained on the next run
>>>>>> # Format is XDG_xxx_DIR="$HOME/yyy", where yyy is a shell-escaped
>>>>>> # homedir-relative path, or XDG_xxx_DIR="/yyy", where /yyy is an
>>>>>> # absolute path. No other format is supported.
>>>>>> #
>>>>>> XDG_DESKTOP_DIR="$HOME/Skrivbord"
>>>>>> XDG_DOWNLOAD_DIR="$HOME/Hämtningar"
>>>>>> XDG_TEMPLATES_DIR="$HOME/Mallar"
>>>>>> XDG_PUBLICSHARE_DIR="$HOME/Publikt"
>>>>>> XDG_DOCUMENTS_DIR="$HOME/Dokument"
>>>>>> XDG_MUSIC_DIR="$HOME/Musik"
>>>>>> XDG_PICTURES_DIR="$HOME/Bilder"
>>>>>> XDG_VIDEOS_DIR="$HOME/Video"
>>>>>>
>>>>>
>>>>> I see.. This is very valid point and I have hoped to hear something like this.
>>>>> Do you think it is correct to get this information from ~/.config/user-dirs.dirs ?
>>>>> It sound good enough to me to parse it... As it is standartized in freedesktop.org.
>>>>>
>>>>> Thanx for reply,
>>>>>
>>>>> J.
>>>>
>>>> So here is improved version which is parsing fro ~/.config/user-dirs.dirs.
>>>>
>>>> J.
>>>
>>>
>>> This is little bit better and is taking care for all possible variables. Instead all this parsing
>>> and substitutions simple exec of echo $(xdg-user-dir DESKTOP) is possible. And it maybe more
>>> bullet proof. However I would like avoid Sytem.exec wheres possible.
>>>
>>> J.
>>>
>>
>> thanks, comments inline
>>
>>> diff -r a16da8b96a0f netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java
> ...
>>> return iconSize;
>>> }
>>>
>>> + public File getShortcutTmpFile() {
>>> + File shortcutFile = new
>>> File(JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_TMP_DIR) +
>>> File.separator + FileUtils.sanitizeFileName(file.getTitle()) + ".desktop");
>>
>> bit of a mouthful, a temporary variable here for the user directory would be good.
>
> as you wish
>
>>
>>> + return shortcutFile;
>>> + }
>>> +
>>> /**
>>> * Set the icon size to use for the desktop shortcut
>>> *
>>> @@ -148,9 +162,7 @@
>>> * Install this XDesktopEntry into the user's desktop as a launcher
>>> */
>>> private void installDesktopLauncher() {
>>> - File shortcutFile = new File(JNLPRuntime.getConfiguration()
>>> - .getProperty(DeploymentConfiguration.KEY_USER_TMP_DIR)
>>> - + File.separator + FileUtils.sanitizeFileName(file.getTitle()) + ".desktop");
>>> + File shortcutFile = getShortcutTmpFile();
>>> try {
>>>
>>> if (!shortcutFile.getParentFile().isDirectory() &&
>>> !shortcutFile.getParentFile().mkdirs()) {
>>> @@ -234,4 +246,80 @@
>>> }
>>> }
>>>
>>> + public String getDesktopIconFinalName() {
>>
>
> as you wish :)
>
>> I don't understand what is 'final' about it ? Why can it simply not be called the icon name?
>>
>>> + return sanitize(file.getTitle());
>>> + }
>>> +
>>> + public File getFinalLinuxDesktopIconFile() {
>>
>> Similarly I don't understand what makes this the final linux desktop icon file, vs
>> 'getLinuxDesktopIconFile'
>
> similarly O:)
>
>>
>>> + return new File(findFreedesktopOrgDesktopPathCatch() + getDesktopIconFinalName() +
>>> ".desktop");
>>> + }
>>> +
>>> + public static String findFreedesktopOrgDesktopPathCatch() {
>>
>> I'm all for splitting into many small methods, but I don't know how I feel about
>> findFreedesktopOrgDesktopPathCatch, findFreedesktopOrgDesktopPath, getFreedesktopOrgDesktopPathFrom
>> all being public, separate methods. I think just inlining all the logic into one
>> findFreedesktopOrgDesktopPath() method suffices, also the name 'findFreedesktopOrgDesktopPathCatch'
>> says too much about the implementation IMO.
>>
>>> + try {
>>> + return findFreedesktopOrgDesktopPath();
>>> + } catch (Exception ex) {
>>
>> I think catching Exception is (almost) always a bad idea, it can silence even array-out-of-bounds. I
>> think IOException suffices.
>
> I really would like to have this cas safe block. However I have added printStackTrace.
>
>>
>>> + return System.getProperty("user.home") + "/Desktop/";
>>> + }
>>> + }
>>> +
>>> + public static String findFreedesktopOrgDesktopPath() throws IOException {
>>> + File userDirs = new File(System.getProperty("user.home") + "/.config/user-dirs.dirs");
>>> + if (!userDirs.exists()) {
>>> + return System.getProperty("user.home") + "/Desktop/";
>>> + }
>>> + return getFreedesktopOrgDesktopPathFrom(userDirs);
>>> + }
>>> +
>>> + public static String getFreedesktopOrgDesktopPathFrom(File userDirs) throws IOException {
>>> + BufferedReader r = new BufferedReader(new FileReader(userDirs));
>>> + try {
>>> + return getFreedesktopOrgDesktopPathFrom(r);
>>> + } finally {
>>> + r.close();
>>> + }
>>> +
>>> + }
>>> +
>>> + public static final String XDG_DESKTOP_DIR = "XDG_DESKTOP_DIR";
>>> +
>>> + public static String getFreedesktopOrgDesktopPathFrom(BufferedReader r) throws IOException {
>>
>> This is a helper method and shouldn't be public IMO.
>
> As much of this methods as possible have been made private. Except this one :)
> To keep it testable I let it package private.
>>
>>> + while (true) {
>>> + String s = r.readLine();
>>> + if (s == null) {
>>> + throw new IOException("End of user-dirs found, but no " + XDG_DESKTOP_DIR + " key
>>> found");
>>> + }
>>> + s = s.trim();
>>> + if (s.startsWith(XDG_DESKTOP_DIR)) {
>>> + if (!s.contains("=")) {
>>> + throw new IOException(XDG_DESKTOP_DIR + " have no value");
>>
>> s/have/has/
>
> thanks.
>
>>
>>> + }
>>> + String[] ss = s.split("=");
>>
>> rename 'ss' to 'keyvalue' or something, and the reason ss[1] is used will be clearer IMO.
>
> done. To keyAndvalue
>>
>>> + ss[1] = ss[1].trim();
>>> + return evaluateVariables(ss[1]);
>>> + }
>>> + }
>>> + }
>>> +
>>> + private static String evaluateVariables(String orig) {
>>> +
>>> + Set<Entry<String, String>> env = System.getenv().entrySet();
>>> + List<Entry<String, String>> envVariables = new ArrayList<Entry<String, String>> (env);
>>> + Collections.sort(envVariables, new Comparator<Entry<String, String>>(){
>>> +
>>> + @Override
>>> + public int compare(Entry<String, String> o1, Entry<String, String> o2) {
>>> + return o2.getKey().length()-o1.getKey().length();
>>> + }
>>> + });
>>> + while (true) {
>>> + String before = orig;
>>> + for (Entry<String, String> entry : envVariables) {
>>> + orig=orig.replaceAll("\\$"+entry.getKey(), entry.getValue());
>>> + }
>>> + if (before.equals(orig)) {
>>> + return orig;
>>> + }
>>> + }
>>> +
>>> + }
>>> }
>>> diff -r a16da8b96a0f tests/netx/unit/net/sourceforge/jnlp/util/XDesktopEntryTest.java
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/XDesktopEntryTest.java Mon Dec 24 21:38:14 2012
>>> +0100
>>> @@ -0,0 +1,59 @@
>>> +/*
>>> + * To change this template, choose Tools | Templates
>>> + * and open the template in the editor.
>>> + */
>>
>> Netbeans user are you ? Anyway this should be gone.
>
> Argh... Added correct header.
>>
>>> +package net.sourceforge.jnlp.util;
>>> +
>>> +import java.io.BufferedReader;
>>> +import java.io.IOException;
>>> +import java.io.StringReader;
>>> +import org.junit.Assert;
>>> +import org.junit.Test;
>>> +
>>> +public class XDesktopEntryTest {
>>> +
>>> + private String des1 = "/my/little/Desktop";
>>> + private String des2name = "Plocha";
>>> + private String des2Res = System.getProperty("user.home") + "/" + des2name;
>>> + private String des2 = "$HOME" + "/" + des2name;
>>> + private String src1 = XDesktopEntry.XDG_DESKTOP_DIR + "=" + des1;
>>> + private String src2 = " " + XDesktopEntry.XDG_DESKTOP_DIR + " = " + des1;
>>> + private String src3 = "#" + XDesktopEntry.XDG_DESKTOP_DIR + " = " + des1;
>>> + private String src4 = XDesktopEntry.XDG_DESKTOP_DIR + "=" + des2;
>>> + private String src5 = " " + XDesktopEntry.XDG_DESKTOP_DIR + " = " + des2;
>>> + private String src6 = "#" + XDesktopEntry.XDG_DESKTOP_DIR + " = " + des2;
>>> +
>>> + @Test
>>> + public void getFreedesktopOrgDesktopPathFromtestSimple() throws IOException {
>>> + String s = XDesktopEntry.getFreedesktopOrgDesktopPathFrom(new BufferedReader(new
>>> StringReader(src1)));
>>> + Assert.assertEquals(s, des1);
>>> + }
>>> +
>>> + @Test
>>> + public void getFreedesktopOrgDesktopPathFromtestSpaced() throws IOException {
>>> + String s = XDesktopEntry.getFreedesktopOrgDesktopPathFrom(new BufferedReader(new
>>> StringReader(src2)));
>>> + Assert.assertEquals(s, des1);
>>> + }
>>> +
>>> + @Test(expected = IOException.class)
>>> + public void getFreedesktopOrgDesktopPathFromtestCommented() throws IOException {
>>> + String s = XDesktopEntry.getFreedesktopOrgDesktopPathFrom(new BufferedReader(new
>>> StringReader(src3)));
>>> + }
>>> +
>>> + @Test
>>> + public void getFreedesktopOrgDesktopPathFromtestSimpleWithHome() throws IOException {
>>> + String s = XDesktopEntry.getFreedesktopOrgDesktopPathFrom(new BufferedReader(new
>>> StringReader(src4)));
>>> + Assert.assertEquals(s, des2Res);
>>> + }
>>> +
>>> + @Test
>>> + public void getFreedesktopOrgDesktopPathFromtestSpacedWithHome() throws IOException {
>>> + String s = XDesktopEntry.getFreedesktopOrgDesktopPathFrom(new BufferedReader(new
>>> StringReader(src5)));
>>> + Assert.assertEquals(s, des2Res);
>>> + }
>>> +
>>> + @Test(expected = IOException.class)
>>> + public void getFreedesktopOrgDesktopPathFromtestCommentedWithHome() throws IOException {
>>> + String s = XDesktopEntry.getFreedesktopOrgDesktopPathFrom(new BufferedReader(new
>>> StringReader(src6)));
>>> + }
>>> +}
>>
>> Test looks OK
>
> of course :)
>
>>
>> Just one additional comment, as Omair pointed out his patch had nice functionality when a desktop
>> icon was deleted (it did not prompt again for desktop icon to be created). It is worth considering.
>
> See the separate sub-thread oook? Especially last record - http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-January/021278.html (Unless Omair will somehow negate it.
>
ping?
> Thank you very much for review!
> J.