[icedtea-web] RFC: PR918: java applet windows uses a low resulution black/white icon

[Omair Majid]

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?

> 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.

> 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
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.

> 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
ImageResources().iconizeWindows(Window w).

> hmhm - it was cached :)

Doh! I have fixed the new version to do that too.

>> +public class ImageResources {
> As I told, this should be static or provide singleton instance. Also 
> imageicon, however small should be cahced.

Done now.

> I would suggest IconLoadingException as it have no other usages
> rigth now.
> 

I am not entirely clear what you are suggesting here. Are you suggesting
an alternate name for the class? I went with ImageLoadingException as
the main class is *Image*Resources :)

>> +    public static class ImageLoadingException

> Why this? isnt Runtime exception enough?  If you insists Why not 
> public class in separate file?

I wanted something more specific to this class. Do you see any issues
with subclassing RuntimeException here? I didn't think this was general
enough for other classes to use yet, so I didn't put it in a separate file.

Anyway, I have removed this class now (see below), so the discussion is
moot :)

>> + +    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).

> 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.

> (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.

> 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?

Thanks,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: let-programs-have-default-icons-02.patch
Type: text/x-patch
Size: 17990 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120418/9230209c/let-programs-have-default-icons-02.patch