[icedtea-web] RFC: PR918: java applet windows uses a low resulution black/white icon
Jiri Vanek
jvanek at redhat.com
Thu Apr 19 03:08:10 PDT 2012
On 04/19/2012 05:22 AM, Omair Majid wrote:
> On 04/18/2012 12:32 PM, Jiri Vanek wrote:
>> On 04/18/2012 05:55 PM, Omair Majid wrote:
>>> On 04/18/2012 11:46 AM, Jiri Vanek wrote:
>>>
>>> Yes, I can certainly modify ImageResources to take care of more
>>> sizes.
>>
>> Then there rises question when to use which :)
>
> Eh? We can just set them all (using setIconImages) and let the window
> manager pick whatever it wants! Or do you mean at places other than the
> window icon?
Yes this will probably make the best deal. And is it one more reason to hide the setting of icons
(as already discussed lower). Or to use setIconImages already now and so prevent future refactoring.
>
>> Btw - why have you decided to discard netx icon as default one and
>> use java's one as default for "users windows"?
>
> It seems much better to me to let them use the java/duke icon. The
> programs are using the java api and they are running in java. Not sure
> that IcedTea-Web should take claim of that and use an icedtea-web icon.
> Also, it should ensure more consistency between running a simple program
> using command line java and javaws.
>
> I would also claim that this is what the proprietary plugin does, but
> then again, they use the same icon as java.
(More then) fair enough.
>
>> 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 :-(
>
> That's what the older code did (or perhaps it tried to do - I am not
looks more like it just tried.
> sure if it worked in all cases). It changed the icon for all new
> windows. The user could set one after the frame window was created.
>
>> 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.
>
> Good point. I have changed it to a singleton now.
Thank you:)
>
>> 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.
>
> I am not a huge fan of this idea. I don't like the idea of
> ImageReources.iconizeMainWindows(), which would find/modifyy all (or
> even any) window. It's not the right place for that. And I am not sure
> how much code I will save by writing
ok.
> ImageResources().iconizeWindows(Window w).
hmhmh. Cannot imagine more refactoring then setIconImage/setIconImages right now (Oh how I miss my
childhood fantasy :) ) So if we will use setIconImages now, I'm ok.
>
..snipp...
>
>>> + + 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.
>
> Actually I did this intentionally :)
>
> I want to use the classloader (whatever it is) to find the image file.
> It may be that the class loader uses a relative path to resolve it. Or
> it may be that the class loader finds this file in one of the jars in
> the class path. Either way, I want to be able to run this without
> assuming our class is loaded by the system classloader. This allows this
> test to be run using an IDE too. It will also allow icedtea-web to work
> when netx.jar is not in the boot classpath. This is basically the
> pattern that is used throughout netx; there are other examples in netx
> code too (the plugin does things differently).
Well if this is pattern then I'm ok. However I still consider it wrong and cannot imagine how this
can be better then simple ClassLoader.getSystemClassLoader() (which is working in ide, on
boot/classpath...)
>
>> 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.
>
> Hm.. you have a valid point. While it's good for developers to see a
> problem early (and missing icons is almost certainly a bug caused by an
> icedtea-web developer), you are right that the program shouldn't fail. I
> have changed it to return a null Image. null Images are handled properly
> (read: ignored) by java so things continue to work if the image is missing.
>
Excelent!
>> (Although the possibility of failure is very small, and there is
>> always point of view to die earlier then later)
>
> Yeah, I was going for die-early. But that's what the unit test is for :)
>
>> :)) And who will test if they have really setted to windows? O:)
>
> I am not sure how to do that. I feel we need more points in netx where
> we can inject objects to write unit tests without going crazy.
>
ok.
>> Thanx for nice fix and one of the best tests I have ever seen :)
>
> I really appreciate the review!
>
> How does the attached (updated) patch look?
Much better:) Few comments inline but you can go ahead without accepting them.
>
> let-programs-have-default-icons-02.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(ImageResources.INSTANCE.getApplicationImage());
for all those lines I would like to use setIconImages(ImageResources.INSTANCE.getApplicationImages());
> +
...snip...
>+ public enum ImageResources {
Wou I have never seen this :)
I'm used for pattern like:
class Class{
private static final Class INSTANCE=new Class();
public static Class getInstance(){retrun INSTANCE;}
}
But I like this enum approach. By my lack of knowledge the advantage(but also disadvantge compared
with yours) of above is method returning the singleton. But I really like your approach! Can you
tell me (just from curiosity) what you consider as advantages for your enum?
> +
> + INSTANCE;
> +
> + private static final String APPLICATION_ICON_PATH =
"net/sourceforge/jnlp/resources/netx-icon.png";
> +
> + private final Map<String, Image> cache = new HashMap<String, Image>();
> +
> + private ImageResources() {}
> +
> + /* this is for testing ONLY */
> + void clearCache() {
> + cache.clear();
> + }
> +
> + /**
> + * Returns an appropriate image, or null if there are errors loading the image.
> + */
Can we agree on return List<Image> right now returning just one one-member-list?
> + public Image getApplicationImage() {
> + if (cache.containsKey(APPLICATION_ICON_PATH)) {
> + return cache.get(APPLICATION_ICON_PATH);
> + }
> +
> + ClassLoader cl = this.getClass().getClassLoader();
> + if (cl == null) {
> + cl = ClassLoader.getSystemClassLoader();
> + }
> +
> + InputStream in = cl.getResourceAsStream(APPLICATION_ICON_PATH);
> + try {
> + Image image = ImageIO.read(in);
> + cache.put(APPLICATION_ICON_PATH, image);
> + return image;
> + } catch (IOException ioe) {
> + ioe.printStackTrace();
> + return null;
> + }
> + }
> +}
> 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;
...snip...
Everything else looks ok.
J.
More information about the distro-pkg-dev
mailing list