[rfc][icedtea-web] fixinf of desktop icon behaviour
Jiri Vanek
jvanek at redhat.com
Wed Jan 2 09:14:28 PST 2013
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.
Thank you very much for review!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedRecreationOfDesktopIcon-0004.patch
Type: text/x-patch
Size: 10990 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130102/6cc08f35/fixedRecreationOfDesktopIcon-0004.patch
More information about the distro-pkg-dev
mailing list