[rfc][icedtea-web] menu support
Jiri Vanek
jvanek at redhat.com
Sat Nov 29 17:31:00 UTC 2014
On 11/27/2014 03:25 PM, Jie Kang wrote:
>
> ----- Original Message -----
>> >On 11/14/2014 02:21 PM, Jiri Vanek wrote:
>>> > >On 11/13/2014 06:17 PM, Jiri Vanek wrote:
>>>> > >>On 11/13/2014 05:59 PM, Jiri Vanek wrote:
>>>>> > >>>Hi!
>>>>> > >>>
>>>>> > >>>This is support for XDG menus.
>>>>> > >>>
>>>>> > >>>Work can be divided into two bug parts
>>>>> > >>> 1) additional features to "install desktop icon" dialogue
>>>>> > >>> 2) install the menu item
>>>>> > >>>
>>>>> > >>>2:
>>>>> > >>>Tested in KDE, Gnome3, mate, xfce. (gnome 2 must-to-do) - see javadoc in
>>>>> > >>>XdesktopEntry.java for some
>>>>> > >>>interesting differences. But works surprisingly well.
>>>>> > >>>
>>>>> > >>>1) our glorious accessdialogue is returning only int. I had to rape it a
>>>>> > >>>bit:( The only other
>>>>> > >>>workaround is to separate desktops's creation to separate class.
>>>>> > >>>COnsidering the many logic in
>>>>> > >>>AccessWarningPane I had inclined to those few if lines (anyway there is
>>>>> > >>>only small possible
>>>>> > >>>workaroound around this "rape 70,40,30,20,0 codes :-/)
>>>>> > >>>
>>>>> > >>>Happy reading :))
>>>>> > >>>
>>>>> > >>>
>>>>> > >>>J.
>>>>> > >>>
>>>>> > >>>Pleas enote, that [rfc][icedtea-web] ignore npe in pac,a nd fixing
>>>>> > >>>--Xoffliner are also part of this
>>>>> > >>>patch. I will remove them in next rounds/before push (As the fixxes were
>>>>> > >>>already approved anyway).
>>>>> > >>>
>>>>> > >>>
>>>>> > >>>After this is in/during reviw, I will work on new panel to
>>>>> > >>>itweb-settings, which allows removing of
>>>>> > >>>icons and menu entries added by javaws.itweb.
>>>>> > >>>
>>>>> > >>>
>>>>> > >>>General thoughs?
>>>>> > >>>
>>>>> > >>>J.
>>>> > >>
>>>> > >>Jsut realized an NPE - in case of ALWAYS_ASK, the information, and
>>>> > >>getShortcut may be null...
>>>> > >>
>>>> > >>
>>>> > >>J.
>>>> > >>
>>> > >just adapted to head.. Hei I like this menu +offline support :))
>> >
>> >
>> >Fixed few messages - thay will go to properties at the end anyway. Also I
>> >have chnaged default icon
>> >from java to javaws. My system dont know javaicon, but knows javaws (wellif
>> >you run jnlp you have to
>> >installe dbooth java, and javaws, but javaws icon is under itw maintainance)
>> >
>> >Actually - I have this line in fedora specfile:
>> > cp javaws.png $RPM_BUILD_ROOT%{_datadir}/pixmaps
>> >
>> >Shouldnt our make install do so? (afaik yes)
>
>
> Hello,
>
>
> First: Pretty awesome work!!! :D
>
>
>
> User-testing on F20 it seems fine, the shorcuts get created and seem to be coded properly. However, not all applets are able to launched through shortcut so it's kind of iffy there.*shrugs*
>
> When testing:http://jogamp.org/deployment/jogamp-next/jogl-applet-version-napplet.html
>
> I was able to create a shortcut for it, and it ended up being a shortcut to an html file. However, when I launched it, it was not opened in my default browser, instead it was attempted to be opened in javaws :\ I think this might be a bug. If it opened in firefox, it would've worked perfectly.
I think I have explained this in previous emial, but I have now included the main logic to this pathc - if it is htmled-applet what is shortcuted, then it opens in browser.
>
>
>
> When testing:http://www.arbores.ca/PresentFuture.jnlp
>
> I encountered:
>
> java.io.FileNotFoundException:/home/jkang/.local/share/applications/javaws/Present/Future Value Calculator.desktop (No such file or directory)
> at java.io.FileOutputStream.open(Native Method)
> at java.io.FileOutputStream.<init>(FileOutputStream.java:221)
> at java.io.FileOutputStream.<init>(FileOutputStream.java:171)
> at net.sourceforge.jnlp.util.XDesktopEntry.installMenuLauncher(XDesktopEntry.java:247)
> at net.sourceforge.jnlp.util.XDesktopEntry.createDesktopShortcuts(XDesktopEntry.java:232)
> at net.sourceforge.jnlp.runtime.ApplicationInstance.addMenuAndDesktopEntries(ApplicationInstance.java:179)
> at net.sourceforge.jnlp.runtime.ApplicationInstance.initialize(ApplicationInstance.java:144)
> at net.sourceforge.jnlp.Launcher.launchApplication(Launcher.java:531)
> at net.sourceforge.jnlp.Launcher$TgThread.run(Launcher.java:912)
>
> Not entirely sure what happened, but the shortcut didn't get created :(
>
>
I think it is quite obvious :)) the title is blah1/blah2 blah3, so instead of file "blah1/blah2 blah3" it was searching fo file "blah2 blah3" in directory blah1 :))
Good catch, fixed.
>
>
> There are a few nits for the code:
>
> Biggest one for me is: Please add more logging messages to make it easier to debug later on: e.g. where shortcut is being saved, etc.
>
done
> Smallest one is: There are a few typos, please try to fix them
I ahve fixe dwhat I found. Not much actually:(( Feel free to name them.
>
>
> Actual code comments:
>
> diff -r e57ab037cbd0 netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java
> --- a/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Fri Nov 14 14:06:06 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Fri Nov 14 15:16:15 2014 +0100
> [...]
>
> - private int[] VALID_ICON_SIZES = new int[] { 16, 22, 32, 48, 64, 128 };
> + private final int[] VALID_ICON_SIZES = new int[] { 16, 22, 32, 48, 64, 128 };
>
> Please add a comment saying what unit these sizes are (I'm guessing pixels?)
added.
>
>
> + public Reader getContentsAsReader(boolean menu) {
>
> String cacheDir = JNLPRuntime.getConfiguration()
> .getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> File cacheFile = CacheUtil.getCacheFile(file.getSourceLocation(), null);
>
> Both cacheDir and cacheFile are now unused, please remove these two lines.
Grate! Thanx for this. Removed.
>
>
> + private static class IconsCreationDescriptor{
> +
> [...]
> + public boolean isMenu() {
> + return menu;
> + }
> +
> +
> +
> +
>
> These extra spaces should be removed;
done
>
>
> diff -r e57ab037cbd0 netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> --- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Nov 14 14:06:06 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Nov 14 15:16:15 2014 +0100
> [...]
> + private void cacheIcon() throws IOException {
> [...]
> + String origLocation = location.substring("file:".length());
>
> Spacing here needs to be fixed
done.
>
>
> + private static String findAndVerifyJavawsMenuDir() {
> + File f = PathsAndFiles.MENUS_DIR.getFile();
> + String ff = f.getAbsolutePath();
>
> Please rename String ff to something like 'path' or 'menuDir', something that says what the string is representing;
>
done
>
> private static String evaluateLinuxVariables(String orig, Map<String, String> variables) {
> Set<Entry<String, String>> env = variables.entrySet();
> - List<Entry<String, String>> envVariables = new ArrayList<Entry<String, String>>(env);
>
> This could be shortened to:
>
> - Set<Entry<String, String>> env = variables.entrySet();
> + List<Entry<String, String>> envVariables = new ArrayList<>(variables.entrySet());
>
> I don't see 'env' being used anywhere else and I don't think the extra line to be specific is useful.
>
sure, done.
>
> diff -r e57ab037cbd0 netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java
> --- a/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Fri Nov 14 14:06:06 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Fri Nov 14 15:16:15 2014 +0100
> @@ -141,11 +141,6 @@
> [...]
> + File possibleMenuFile = entry.getLinuxMenuIconFile();
> + //if one of menu or desktop exists, do not bother user
> + boolean exists = false;
>
> If I am reading this right, I think this is a pretty annoying flaw. If a user creates either a menu or desktop shortcut, they can no longer create a shortcut of the other type? That sounds bad to me, can this be reworked?
Nope - this is intedet exactly to be like this. Can you imagine, dialog with two options "create 1 or/and 2" you select only one of it, and it will keep you bothering untill you select both of tehm.\
Right nwow I would really like to keep it as it is. Once the itweb-support will be done,. may this will give more sense to you.
>
> + boolean trullyTrue = (sd != null && (sd.onDesktop() || sd.getMenu() != null));
>
> Please find a better name for this boolean. What does it even check for?
Actually it appeared it was wrong. See new impl. Lucky catch :)
>
>
> diff -r e57ab037cbd0 netx/net/sourceforge/jnlp/config/PathsAndFiles.java
> --- a/netx/net/sourceforge/jnlp/config/PathsAndFiles.java Fri Nov 14 14:06:06 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/config/PathsAndFiles.java Fri Nov 14 15:16:15 2014 +0100
> @@ -53,6 +53,7 @@
> [...]
> + private static final String DATA_HOME;
>
> I am not sure this is the best name. What is DATA? Atm this name is way too generic, and either needs a comment or renaming.
I'm afraid it is the best neame - it is exact conversion of XDG_DATA_HOME. And as we do everywhere we fill SOMETHING by value of XDG_SOMETHING or default if XDG_* is null.
ok with this explanation?
Thank you for review!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: menus_5.patch
Type: text/x-patch
Size: 46495 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141129/ff7766d3/menus_5-0001.patch>
More information about the distro-pkg-dev
mailing list