[rfc][icedtea-web] fixinf of desktop icon behaviour
Adam Domurad
adomurad at redhat.com
Wed Jan 9 06:42:31 PST 2013
On 01/02/2013 12:14 PM, Jiri Vanek wrote:
> [..snip..]
>>>
>>
>> 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 :)
Thanks.
>
>> 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.
Yep.
>>
>>> + 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.
>> [..snip..]
>
>>
>> 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.
Noted. Patch is OK as is.
>
> Thank you very much for review!
> J.
Thanks for update.
> diff -r a16da8b96a0f
> netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java
> --- a/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Mon
> Dec 24 13:58:31 2012 +0100
> +++ b/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Wed
> Jan 02 18:13:30 2013 +0100
> @@ -17,6 +17,7 @@
> package net.sourceforge.jnlp.runtime;
>
> import java.awt.Window;
> +import java.io.File;
> import java.net.URL;
> import java.security.AccessControlContext;
> import java.security.AccessController;
> @@ -146,7 +147,14 @@
> private void addMenuAndDesktopEntries() {
> XDesktopEntry entry = new XDesktopEntry(file);
> ShortcutDesc sd = file.getInformation().getShortcut();
> -
> + File possibleDesktopFile = entry.getLinuxDesktopIconFile();
> + if (possibleDesktopFile.exists()) {
> + if (JNLPRuntime.isDebug()) {
> + System.out.println("ApplicationInstance.addMenuAndDesktopEntries():
> file - "
> + + possibleDesktopFile.getAbsolutePath() + "
> already exists. Not proceeding with desktop additions");
> + }
> + return;
> + }
> if (shouldCreateShortcut(sd)) {
> entry.createDesktopShortcut();
> }
> diff -r a16da8b96a0f netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> --- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Mon Dec 24
> 13:58:31 2012 +0100
> +++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Wed Jan 02
> 18:13:30 2013 +0100
> @@ -16,16 +16,24 @@
>
> package net.sourceforge.jnlp.util;
>
> +import java.io.BufferedReader;
> import java.io.File;
> import java.io.FileNotFoundException;
> import java.io.FileOutputStream;
> +import java.io.FileReader;
> import java.io.IOException;
> import java.io.OutputStreamWriter;
> import java.io.Reader;
> import java.io.StringReader;
> import java.net.URL;
> import java.nio.charset.Charset;
> +import java.util.ArrayList;
> import java.util.Arrays;
> +import java.util.Collections;
> +import java.util.Comparator;
> +import java.util.List;
> +import java.util.Map.Entry;
> +import java.util.Set;
>
> import net.sourceforge.jnlp.IconDesc;
> import net.sourceforge.jnlp.JNLPFile;
> @@ -48,6 +56,7 @@
>
> public static final String JAVA_ICON_NAME = "java";
>
> +
Nit: extra line
> private JNLPFile file = null;
> private int iconSize = -1;
> private String iconLocation = null;
> @@ -78,7 +87,7 @@
>
> String fileContents = "[Desktop Entry]\n";
> fileContents += "Version=1.0\n";
> - fileContents += "Name=" + sanitize(file.getTitle()) + "\n";
> + fileContents += "Name=" + getDesktopIconName() + "\n";
> fileContents += "GenericName=Java Web Start Application\n";
> fileContents += "Comment=" +
> sanitize(file.getInformation().getDescription()) + "\n";
> fileContents += "Type=Application\n";
> @@ -122,6 +131,12 @@
> return iconSize;
> }
>
> + public File getShortcutTmpFile() {
> + String userTmp =
> JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_TMP_DIR);
> + File shortcutFile = new File(userTmp + File.separator +
> FileUtils.sanitizeFileName(file.getTitle()) + ".desktop");
> + return shortcutFile;
> + }
> +
> /**
> * Set the icon size to use for the desktop shortcut
> *
> @@ -148,9 +163,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 +247,81 @@
> }
> }
>
> + public String getDesktopIconName() {
> + return sanitize(file.getTitle());
> + }
> +
> + public File getLinuxDesktopIconFile() {
> + return new File(findFreedesktopOrgDesktopPathCatch() +
> getDesktopIconName() + ".desktop");
> + }
> +
> + private static String findFreedesktopOrgDesktopPathCatch() {
> + try {
> + return findFreedesktopOrgDesktopPath();
> + } catch (Exception ex) {
> + ex.printStackTrace();
OK, fair enough. (catching Exception still evil IMO)
> + return System.getProperty("user.home") + "/Desktop/";
> + }
> + }
> +
> + private 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);
> + }
> +
> + private static String getFreedesktopOrgDesktopPathFrom(File
> userDirs) throws IOException {
> + BufferedReader r = new BufferedReader(new FileReader(userDirs));
> + try {
> + return getFreedesktopOrgDesktopPathFrom(r);
> + } finally {
> + r.close();
> + }
> +
> + }
> +
> + static final String XDG_DESKTOP_DIR = "XDG_DESKTOP_DIR";
> +
> + static String getFreedesktopOrgDesktopPathFrom(BufferedReader r)
> throws IOException {
> + 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 + " has no
> value");
> + }
> + String[] keyAndValue = s.split("=");
> + keyAndValue[1] = keyAndValue[1].trim();
> + return evaluateLinuxVariables(keyAndValue[1]);
> + }
> + }
> + }
> +
> + private static String evaluateLinuxVariables(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
> Wed Jan 02 18:13:30 2013 +0100
> @@ -0,0 +1,91 @@
> +/*
> +Copyright (C) 2012 Red Hat, Inc.
> +
> +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.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)));
> + }
> +}
OK for HEAD, thanks for the solution.
-Adam
More information about the distro-pkg-dev
mailing list