[rfc][icedtea-web] menu support
Jiri Vanek
jvanek at redhat.com
Fri Dec 5 17:04:21 UTC 2014
On 12/01/2014 08:58 PM, Jie Kang wrote:
>
> ----- Original Message -----
>> >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.
> Okay.
>
>> >
>>> > >
>>> > >+ 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?
>
> Yeah, explanation makes sense.
>
>
>
> Code-nits:
>
>
> diff -r e0e13a1fb240 netx/net/sourceforge/jnlp/ShortcutDesc.java
> --- a/netx/net/sourceforge/jnlp/ShortcutDesc.java Fri Nov 28 11:22:05 2014 -0500
> +++ b/netx/net/sourceforge/jnlp/ShortcutDesc.java Sat Nov 29 17:49:47 2014 +0100
> @@ -16,6 +16,9 @@
> [...]
> + * based on exitence of menu tag
> existence
> + * if null, then no tag was presented
> + * if there si some value, then menu tag was presented
> is
> + * depnding on value inside MenuDesc, the attribute submenu was/was not presented
> depending
>
> + public String getSystemPathStubAcronym() {
> + return VARIABLE + "" + XDG_DATA_HOME;
> + }
>
> What is the reason for the '+ "" +' ? Should it be " " instead? Otherwise I think it should be removed.
>
>
> diff -r e0e13a1fb240 netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java
> --- a/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Fri Nov 28 11:22:05 2014 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java Sat Nov 29 17:49:47 2014 +0100
> [...]
> + public IconsCreationDescriptor(boolean whoCares) {
> + this(whoCares, whoCares);
> + }
>
> I don't think this is the best name for the boolean.. As well I'd rather have the caller use the constructor IconsCreationDescriptor(boolean desktop, boolean menu) : it is more explicit to the code-reader. This single-arg constructor seems to be just out of convenience, not really necessary.
>
> diff -r e0e13a1fb240 netx/net/sourceforge/jnlp/util/FileUtils.java
> --- a/netx/net/sourceforge/jnlp/util/FileUtils.java Fri Nov 28 11:22:05 2014 -0500
> +++ b/netx/net/sourceforge/jnlp/util/FileUtils.java Sat Nov 29 17:49:47 2014 +0100
> @@ -77,7 +77,7 @@
> [...]
> - public static String sanitizePath(String path) {
> + public static String sanitizePath(String path) {
>
> Spacing here seems off.
>
> + return sanitizePath(path, SANITIZED_CHAR);
> + }
>
>
> diff -r e0e13a1fb240 netx/net/sourceforge/jnlp/util/XDesktopEntry.java
> --- a/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Fri Nov 28 11:22:05 2014 -0500
> +++ b/netx/net/sourceforge/jnlp/util/XDesktopEntry.java Sat Nov 29 17:49:47 2014 +0100
> @@ -28,6 +28,8 @@
> [...]
> /* key=value pairs must be a single line */
> + input = FileUtils.sanitizeFileName(input, '-');
> + //return first line or replace new lines by space?
> return input.split("\n")[0];
>
> Spacing here seems off as well.
>
> [...]
> - public void createDesktopShortcut() {
> + public void createDesktopShortcuts(boolean menu, boolean desktop) {
> try {
> + if (menu || desktop) {
> cacheIcon();
> - installDesktopLauncher();
> + }
> + if (desktop){
> + installDesktopLauncher();
> + }
> + if (menu){
> + installMenuLauncher();
> + }
> } catch (Exception e) {
> OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
> }
> }
> +
> +
>
> Extra space?
>
> + private static String findAndVerifyJavawsMenuDir() {
> + File f = PathsAndFiles.MENUS_DIR.getFile();
> + String fPath = f.getAbsolutePath();
> + if (!f.exists()){
> + if (!f.mkdirs()){
> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL, fPath + " directroy for stroing menu entry cannot be created. You may expect failure");
> + }
> + };
> + return fPath;
> + }
> +
> +
> +
> +
> +
>
> Extra spaces here as well
>
>
>
>
> Apart from above the code looks great. I am still manual testing but if anything pops up, you can always do another patch on top :)
>
>
>
> Regards,
>
>> >
>> >
>> >Thank you for review!
>> >
>> > J.
>> >
> -- Jie Kang
>
hi!
Thank you for review. Pushing now with all fixed, except naming/usage in/of public
IconsCreationDescriptor(boolean whoCares)
Because I'm removing this class completely in next changeset.
The return VARIABLE + "" + XDG_DATA_HOME; is little bit more spread. I have kept it as it was. The
reason for original appearance was readability. Feel free to fix in separate changeset.
Also I noted that there is probably bug.
on linux, VARIABLE + "" + XDG_DATA_HOME; expand to $XDG_DATA_HOME; which is correct.
However on windows,
%XDG_DATA_HOME; which.. I dont know, but shouldnt it be %XDG_DATA_HOME% ? If so , it should be
included in this patch too....
again, tyvm!
J
More information about the distro-pkg-dev
mailing list