[rfc][icedtea-web] menu support
Jie Kang
jkang at redhat.com
Mon Dec 1 19:58:49 UTC 2014
----- 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
More information about the distro-pkg-dev
mailing list