[icedtea-web] RFC: PR918: java applet windows uses a low resulution black/white icon
Jiri Vanek
jvanek at redhat.com
Wed Apr 18 09:32:04 PDT 2012
On 04/18/2012 05:55 PM, Omair Majid wrote:
> Hi Jiri,
>
> On 04/18/2012 11:46 AM, Jiri Vanek wrote:
>> On 04/18/2012 04:57 PM, Omair Majid wrote:
>> Walking through the patch, but I will reply tomorrow after full
>> exploration.
>
> Thanks in advance!
>
>>>
>>> The attached patch fixes PR918 [1] by removing the code the set the
>>> icons for all top-level windows. Any windows created by an applet/jnlp
>>> application should now get the default java icon if they dont set an
>>> icon explicitly. The patch also sets the icons for all the top-level
>>> windows we create explicitly to the default application icon (currently
>>> netx's icon).
>>>
>>> We probably want to change the application icon from netx-icon.png to
>>> something else too, but I plan to tackle that as a separate issue (do we
>>> even have an icedtea-web icon?)
>>>
>>> Any thoughts or comments?
>>>
>> Yap O:) - after first look the usage of Nx icon have terrified me :)
>> [nothing personal, it is just small and nasty;)
>>
>
> As I said, I am completely open to replacing it. The patch just makes
> sure that the icon is only used for icedtea-web windows as opposed to
> all windows by default.
>
>> Just hint for icon - IMHO it should be similar to splash screen.
>
> Yes, some sort of consistency would be great.
>
>> On
>> one side i can re factor splash screen design to match new icon. On
>> second new icon can be inspired by splash screen.
>> so 16x16 I and W letters, some color matches
>> 32x32 IcedTea-web as my small splashscreen is designed
>> 64x64 Just shrunken splashscreen - full colours and leafs
>>
>> This sizes are just idea
>
> I am not a graphics expert, so I will be okay with whatever new
> size/shape the new icon takes (and whoever the new designer happens to
> be ;) ).
>
>> and your ImageResources can provide small x
>> medium x large icon as desired by moment and situation.
>>
>
> Yes, I can certainly modify ImageResources to take care of more sizes.
Then there rises question when to use which :)
Btw - why have you decided to discard netx icon as default one and use java's one as default for
"users windows"?
In case icons will be little bit nicer, then it sounds to me like good idea to have this icons
everywhere.
Can we determine if user ave specified his own? I' afraid we can not :-(
Just what I have seen for now:
You are using on everywhere new Imageresource().getApplicationImage()
It is not nice.
It should be static or singleton. My vote is for singleton. Both will allow caching, and singleton
is "more object like".
the choice between ImageResources.getInstance().getApplicationImage() and
ImageResources.getApplicationImage() is up to you but I do not like creating new ImageResources()
with new icon every time.
Also considering on how much places you have to setIconImage(new
ImageResources().getApplicationImage()); (or similar) I'm thinking about pack it somewhere. But I'm
not sure how and where.
eg new ImageResources().iconizeMainWindows()?
or etleast eg new ImageResources().iconizeWindows(Window w)? (but only the first one looks like
have sense if possible) And also such a "magical method" can be misleading for future developers.
Few more hints inline:
> let-programs-have-default-icons-01.patch
>
>
> diff --git a/ChangeLog b/ChangeLog
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,33 @@
> +2012-04-18 Omair Majid<omajid at redhat.com>
> +
> + PR918: java applet windows uses a low resulution black/white icon
> + * NEWS: Update with fix.
> + * netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java: Remove windowIcon.
> + (initialize): Do not call loadWindowIcon.
> + (getWindowIcon): Remove.
> + (setWindowIcon): Remove.
> + (loadWindowIcon): Remove.
> + * netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> + (checkTopLevelWindow): Do not set the icon for all top level windows. Use
> + the default java icon instead.
> + * netx/net/sourceforge/jnlp/util/ImageResources.java: New file. Provides
> + access to icons.
> + * netx/net/sourceforge/jnlp/JNLPSplashScreen.java (JNLPSplashScreen),
> + * netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java
> + (getListener),
> + * netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> + (AdvancedProxySettingsDialog),
> + * netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java (CacheViewer),
> + * netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java (ControlPanel),
> + * netx/net/sourceforge/jnlp/security/SecurityDialog.java (SecurityDialog),
> + * netx/net/sourceforge/jnlp/security/viewer/CertificateViewer.java
> + (CertificateViewer),
> + * netx/net/sourceforge/jnlp/util/BasicExceptionDialog.java (show),
> + * plugin/icedteanp/java/sun/applet/JavaConsole.java (initialize):
> + Explicitly load icons.
> + * tests/netx/unit/net/sourceforge/jnlp/util/ImageResourcesTest.java: Test
> + for ImageResources class.
> +
> 2012-04-18 Jiri Vanek<jvanek at redhat.com>
>
> Allowed signed applets in automatic reproducers tests
> diff --git a/NEWS b/NEWS
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,8 @@
> * Plugin
> - PR820: IcedTea-Web 1.1.3 crashing Firefox when loading Citrix XenApp
> - PR895: IcedTea-Web searches for missing classes on each loadClass or findClass
> +* Common
> + - PR918: java applet windows uses a low resulution black/white icon
>
> New in release 1.2 (2011-XX-XX):
> * Security updates:
> diff --git a/netx/net/sourceforge/jnlp/JNLPSplashScreen.java
b/netx/net/sourceforge/jnlp/JNLPSplashScreen.java
> --- a/netx/net/sourceforge/jnlp/JNLPSplashScreen.java
> +++ b/netx/net/sourceforge/jnlp/JNLPSplashScreen.java
> @@ -14,6 +14,7 @@
>
> import net.sourceforge.jnlp.cache.ResourceTracker;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> public class JNLPSplashScreen extends JDialog {
>
> @@ -28,6 +29,8 @@
> public JNLPSplashScreen(ResourceTracker resourceTracker,
> String applicationTitle, String applicationVendor) {
>
> + setIconImage(new ImageResources().getApplicationImage());
(1)
> +
> // If the JNLP file does not contain any icon images, the splash image
> // will consist of the application's title and vendor, as taken from the
> // JNLP file.
> diff --git a/netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java
b/netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java
> --- a/netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java
> +++ b/netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java
> @@ -28,6 +28,7 @@
> import javax.jnlp.*;
>
> import net.sourceforge.jnlp.runtime.*;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> /**
> * Show the progress of downloads.
> @@ -101,6 +102,7 @@
> synchronized (frameMutex) {
> if (frame == null) {
> frame = new JFrame(downloading + "...");
> + frame.setIconImage(new ImageResources().getApplicationImage());
(2)
> frame.getContentPane().setLayout(new GridBagLayout());
> }
>
> diff --git a/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> +++ b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> @@ -31,6 +31,7 @@
>
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.Translator;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> /**
> * This dialog provides a means for user to edit more of the proxy settings.
> @@ -54,6 +55,8 @@
> */
> public AdvancedProxySettingsDialog(DeploymentConfiguration config) {
> super((Frame) null, dialogTitle, true); // Don't need a parent.
> + setIconImage(new ImageResources().getApplicationImage());
(3)
> +
> this.config = config;
>
> /* Prepare for adding components to dialog box */
> diff --git a/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
b/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
> +++ b/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
> @@ -30,6 +30,7 @@
>
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.Translator;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> /**
> * This class will provide a visual way of viewing cache.
> @@ -51,6 +52,7 @@
> */
> public CacheViewer(DeploymentConfiguration config) {
> super((Frame) null, dialogTitle, true); // Don't need a parent.
> + setIconImage(new ImageResources().getApplicationImage());
(4)
> this.config = config;
>
> /* Prepare for adding components to dialog box */
> diff --git a/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> +++ b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> @@ -55,6 +55,7 @@
> import net.sourceforge.jnlp.runtime.Translator;
> import net.sourceforge.jnlp.security.KeyStores;
> import net.sourceforge.jnlp.security.viewer.CertificatePane;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> /**
> * This is the control panel for Java. It provides a GUI for modifying the
> @@ -102,6 +103,7 @@
> public ControlPanel(DeploymentConfiguration config) {
> super();
> setTitle(Translator.R("CPHead"));
> + setIconImage(new ImageResources().getApplicationImage());
(5)
>
> this.config = config;
>
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> @@ -94,9 +94,6 @@
> /** update policy that controls when to check for updates */
> private static UpdatePolicy updatePolicy = UpdatePolicy.ALWAYS;
>
> - /** netx window icon */
> - private static Image windowIcon = null;
> -
> /** whether initialized */
> private static boolean initialized = false;
>
> @@ -188,9 +185,6 @@
> if (headless == false)
> checkHeadless();
>
> - if (!headless&& windowIcon == null)
> - loadWindowIcon();
> -
hmhm - it was cached :)
> if (!headless&& indicator == null)
> indicator = new DefaultDownloadIndicator();
>
> @@ -336,24 +330,6 @@
> }
>
> /**
> - * Returns the window icon.
> - */
> - public static Image getWindowIcon() {
> - return windowIcon;
> - }
> -
> - /**
> - * Sets the window icon that is displayed in Java applications
> - * and applets instead of the default Java icon.
> - *
> - * @throws IllegalStateException if caller is not the exit class
> - */
> - public static void setWindowIcon(Image image) {
> - checkExitClass();
> - windowIcon = image;
> - }
> -
> - /**
> * Returns whether the JNLP client will use any AWT/Swing
> * components.
> */
> @@ -622,22 +598,6 @@
> }
>
> /**
> - * Load the window icon.
> - */
> - private static void loadWindowIcon() {
> - if (windowIcon != null)
> - return;
> -
> - try {
> - windowIcon = new javax.swing.ImageIcon((new sun.misc.Launcher())
> -
.getClassLoader().getResource("net/sourceforge/jnlp/resources/netx-icon.png")).getImage();
> - } catch (Exception ex) {
> - if (JNLPRuntime.isDebug())
> - ex.printStackTrace();
> - }
> - }
> -
> - /**
> * @return true if running on Windows
> */
> public static boolean isWindows() {
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPSecurityManager.java
> @@ -350,12 +350,6 @@
> app.addWindow(w);
> }
>
> - // change coffee cup to netx for default icon
> - if (window instanceof Window)
> - for (Window w = (Window) window; w != null; w = w.getOwner())
> - if (window instanceof Frame)
> - ((Frame) window).setIconImage(JNLPRuntime.getWindowIcon());
> -
> // todo: set awt.appletWarning to custom message
> // todo: logo on with glass pane on JFrame/JWindow?
>
> diff --git a/netx/net/sourceforge/jnlp/security/SecurityDialog.java
b/netx/net/sourceforge/jnlp/security/SecurityDialog.java
> --- a/netx/net/sourceforge/jnlp/security/SecurityDialog.java
> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialog.java
> @@ -41,6 +41,7 @@
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> import net.sourceforge.jnlp.security.SecurityDialogs.AccessType;
> import net.sourceforge.jnlp.security.SecurityDialogs.DialogType;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> import java.awt.*;
>
> @@ -98,6 +99,7 @@
> SecurityDialog(DialogType dialogType, AccessType accessType,
> JNLPFile file, CertVerifier JarCertVerifier, X509Certificate cert, Object[]
extras) {
> super();
> + setIconImage(new ImageResources().getApplicationImage());
(6)
> this.dialogType = dialogType;
> this.accessType = accessType;
> this.file = file;
> diff --git a/netx/net/sourceforge/jnlp/security/viewer/CertificateViewer.java
b/netx/net/sourceforge/jnlp/security/viewer/CertificateViewer.java
> --- a/netx/net/sourceforge/jnlp/security/viewer/CertificateViewer.java
> +++ b/netx/net/sourceforge/jnlp/security/viewer/CertificateViewer.java
> @@ -51,6 +51,7 @@
> import javax.swing.UIManager;
>
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> public class CertificateViewer extends JDialog {
>
> @@ -61,6 +62,7 @@
>
> public CertificateViewer() {
> super((Frame) null, dialogTitle, true);
> + setIconImage(new ImageResources().getApplicationImage());
(7)
>
> Container contentPane = getContentPane();
> contentPane.setLayout(new BorderLayout());
> diff --git a/netx/net/sourceforge/jnlp/util/BasicExceptionDialog.java
b/netx/net/sourceforge/jnlp/util/BasicExceptionDialog.java
> --- a/netx/net/sourceforge/jnlp/util/BasicExceptionDialog.java
> +++ b/netx/net/sourceforge/jnlp/util/BasicExceptionDialog.java
> @@ -82,6 +82,7 @@
>
> JOptionPane optionPane = new JOptionPane(mainPanel, JOptionPane.ERROR_MESSAGE);
> final JDialog errorDialog = optionPane.createDialog(R("Error"));
> + errorDialog.setIconImage(new ImageResources().getApplicationImage());
(8)
>
> final JPanel quickInfoPanel = new JPanel();
> BoxLayout layout = new BoxLayout(quickInfoPanel, BoxLayout.Y_AXIS);
> diff --git a/netx/net/sourceforge/jnlp/util/ImageResources.java
b/netx/net/sourceforge/jnlp/util/ImageResources.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/util/ImageResources.java
> @@ -0,0 +1,69 @@
> +/* ImageResources.java
> + 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; either version 2, or (at your option)
> +any later version.
> +
> +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.awt.Image;
> +import java.io.IOException;
> +import java.io.InputStream;
> +
> +import javax.imageio.ImageIO;
> +
> +public class ImageResources {
As I told, this should be static or provide singleton instance.
Also imageicon, however small should be cahced.
> +
I would suggest IconLoadingException as it have no other usages rigth n ow.
> + public static class ImageLoadingException extends RuntimeException {
> + public ImageLoadingException(Throwable cause) {
> + super(cause);
> + }
> + }
Why this? isnt Runtime exception enough? If you insists Why not public class in separate file?
> +
> + private static final String APPLICATION_ICON_PATH =
"net/sourceforge/jnlp/resources/netx-icon.png";
I see an issue in following code. When
this.getClass().getClassLoader() is got, and it is not systemClassLoader then it search for resource
by relative path - so your path will not be valid.
I'm for usage of ClassLoader.getSystemClassLoader() in all ways.
> +
> + public Image getApplicationImage() throws ImageLoadingException {
> + ClassLoader cl = this.getClass().getClassLoader();
> + if (cl == null) {
> + cl = ClassLoader.getSystemClassLoader();
> + }
> + InputStream in = cl.getResourceAsStream(APPLICATION_ICON_PATH);
> + try {
> + Image image = ImageIO.read(in);
> + return image;
> + } catch (IOException ioe) {
> + throw new ImageLoadingException(ioe);
It give me sense to have an special exception, but in this case I'm for wrapping all possible
exceptions. if image is null. I'snt worthy to return eg red square?
Also I think that when this really fails, it should not propagate an exception, (but it should be
printStackTraced in debug mode of course!).
Because if the icons will be just default, then it is better to have default cofe cup rather then fail.
(Although the possibility of failure is very small, and there is always point of view to die earlier
then later)
> + }
> + }
> +}
> diff --git a/plugin/icedteanp/java/sun/applet/JavaConsole.java
b/plugin/icedteanp/java/sun/applet/JavaConsole.java
> --- a/plugin/icedteanp/java/sun/applet/JavaConsole.java
> +++ b/plugin/icedteanp/java/sun/applet/JavaConsole.java
> @@ -65,6 +65,7 @@
>
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.util.ImageResources;
>
> /**
> * A simple Java console for IcedTeaPlugin
> @@ -92,6 +93,7 @@
> final String logDir =
JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_LOG_DIR);
>
> consoleWindow = new JFrame("Java Console");
> + consoleWindow.setIconImage(new ImageResources().getApplicationImage());
(9)
>
> JPanel contentPanel = new JPanel();
> contentPanel.setLayout(new GridBagLayout());
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/util/ImageResourcesTest.java
b/tests/netx/unit/net/sourceforge/jnlp/util/ImageResourcesTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/ImageResourcesTest.java
> @@ -0,0 +1,54 @@
> +/* ImageResourcesTest.java
> + 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; either version 2, or (at your option)
> +any later version.
> +
> +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 static org.junit.Assert.assertNotNull;
> +
> +import java.awt.Image;
> +
> +import org.junit.Test;
> +
> +public class ImageResourcesTest {
> +
> + @Test
> + public void testApplicationIcon() {
> + ImageResources imageResources = new ImageResources();
> + Image icon = imageResources.getApplicationImage();
> + assertNotNull(icon);
> + }
> +}
:)) And who will test if they have really setted to windows? O:)
>
This is simple and straightforward patch doing (except possible classloader and path issue) its job
well. Feel free to smash all my hints (especially "refactoring ones" and "annoying ones" as you
wish.
Thanx for nice fix and one of the best tests I have ever seen :)
Best regards
J.
More information about the distro-pkg-dev
mailing list