Icedtea-web splashscreen implementation

[Omair Majid]

Hi Jiri,

Err...sorry about the delay. I was under the assumption you would be
posting new(er) patches. Thanks for pinging me.

Oh, and please don't take any of this the wrong way; this is great
stuff! I just want it to be better (if possible!).

On 03/23/2012 12:55 PM, Jiri Vanek wrote:
>> Something I am a little curious about is the interaction between the
>> LaunchHandler and the splash screen. I got the feeling that NetX had
>> intended LaunchHandler to be used to indicate progress with the
>> launching of JNLP programs. And that's what I used to implement the
>> splash screens. Perhaps it might be good if we could somehow combine
>> it/split responsibilities. If the launch handler was per
>> application/applet (currently it is shared by the entire runtime), it
>> could be the one to always trigger the (right) splash screen.
>>
> 
> I do not think I understand 100% what you mean. Is it connected with
> ProgessListener ("I used to implement the splash screens") as we were
> talking on IRC when I was designing the splashscreen? (Btw I want to add
> at least interface support for this interface). Or not? (It looks like
> not) But then I do not understand.

There is a class in netx called net.sourceforge.jnlp.LaunchHandler -
that's what I am referring to. It has generically named methods
(launchInitialized, launchStarting, launchCompleted) and it seems like
it would be nice fit for handling splash screens without being
explicitly tied to splash screens. When launchStarting() is invoked, we
could call SplashScreen.show() (sorry if that's not the right method
name) from there (and similarly for the other methods). It's what netx
uses to display splash screens for jnlp applications. The only problem
is that there is only one instance per-process and we would have to
modify to make it per applet.

>> That would mean that applets and jnlps could always rely on informing
>> the launch handler (without knowing what it is) and the launch handler
>> could do the right thing (show dialog, hide splash, whatever).
> 
> If I understand this correctly I had to overlook something. I already
> did the splash-delivering method in one place, and should be easy  (I
> have factory method which is providing (splash)component for applet or
> javaws to be used) to move it to better place. But I can not see this
> common creator as you are describing it.

Yeah, but I fear it's the wrong level of abstraction. Now every place
knows about the SplashUtils class and interacts with it directly, rather
than talking to some generic interface to provide updates.

>>> Test framework was enriched for slow downloading. If the file have
>>> prefix "XslowX" then its downlaoding (even on localhost) will take
>>> 10seconds.
>>
>> I see Pavel has already pointed this out, but please no magic behaviour
>> like this. It will make life hell for the whoever ends up maintaining
>> these in the future.
> I know it is evil, But I was not able to force ALL case I needed it to
> do the same job better.

Okay, how about this: let's tackle this separately. Can you post a patch
for just this and we can try and improve it?

>> I think the "splashscreen" is doing too much. It is drawing the 
>> logo, animating it, showing error messages (and probably a lot
>> more). Can you please split this out?
> Hmhm - true. But not easy task to decomposite it  properly. Those
> tasks are very closely connected and should remain connected
> (especially form point of interface view). I think thy really should
> remains connected, but I agree that at least private decomposition
> should definitely  done. (at least also for unit testing)

No, I think the tasks (error display vs splash) is actually different.
Consider that javaws' error display is in the form of a separate dialog
and for the plugin it's inline. So we actually need multiple
implementations of just those tasks. Combining them in a super-interface
seems a little weird - especially because the error messages are not
related to the splash screen for javaws.

>> I am thinking of something like this (this is only the "view" (swing/awt
>> component) hierarchy:
>>
>> AppletViewerPanel
>>    - Main-Container
>>      - Splash Screen Container
>>      - Error Indicator Container
> 
> I disagree with this  approach. As I wrote, the tasks are very strongly
> connected. Eg splash x error screens are the same, with just different
> color schema.
> 
> I really agree, that individual components should be (privately?)
> extracted, but i would like to keep the event drivern model as it is now
> (getSpalshscreen().spin(),  getSpalshscreen().showError())  rather then
> container().replaceSpalshWithErrorSplash(factory.getDefaultError()) or
> something like this. As it seems to me you are suggesting.

Well, I guess we disagree on this point. I am against leaking
implementation details too - spin() might be better called show() (and
showing it would start the animation (which may be spinning) automatically).

> This is one of the most fundamental parts and I'm definitely looking
> forward for your feedback.
> 
> As a middle way (what is maybe the truth behind your swing/awt example)
> seems to me more and (much more) private decomposition. But to avoid of
> components parts being controlled from outside.
> eg getSpalshscreen().showError() will ionernally do : remove okDrawer
> and add errorDrawer. Which both will be some external classes doing
> their job (but I would like to avoid jcomponents for the if necessary)
> 
> What do you think?

Oh, I definitely agree on the need for private decomposition. But I also
think we need the decomposition to be visible a little more publicly.
There is no reason a splash screen should have anything to do with
errors. That fact that it does, for applets, seems more like an
implementation detail.

>> Basically, each class/object should have one task. And you can swap out
>> the splash screen with the error indicator when an error happens. If you
>> can, please split out the gui-related classes from the non-gui related
>> (the logic) classes.
>>
>>> To forward error to applet is used SplashUtils.showError set of
>>> overloaded methods.
>>
>> I just looked at the implementation of this method. Please avoid using
>> reflection. It just pushes compile time errors into runtime errors, and
>> pollutes method signatures (you know, NoSuchMethodException,
>> IllegalAccessException, IllegalArgumentException,
>> InvocationTargetException). Is
>> there an issue with using a normal interface?
>>
> 
> Definitely! Thanx for kick!
> 
>>> SplashUtils have also factory method which handles
>>> which splash (type and purpose (javaws x applet)) is about to be shown
>>> (if any - see bonuses)
>>
>> Not many places in icedtea-web make an explicit distinction between
>> javaws and applet. Instead of spreading this knowledge around, can you
>> put this in one place? JNLPRuntime seems appropriate to me, but there
>> may be a better place
> 
> Hmmm.. I made quite big effort to minimze the splitting(/or even
> doubling!) of functionality. And except two the calls of factory (one
> form applet, one from javaws) there in none. Can you be little bit more
> specific in your request (or better to point the place where the splash
> could be set uniformly (I do not believe there is such a place!))
> 

Well, that's sort of my point. This isn't specific to the splash screen
at all. If the splash screen needs this, other parts probably do too.
Can we move this to, say, the JNLPRuntime class? IIRC, it already has a
constructor/flag to differentiate applets from jnlp.

>>> There are actually three and half types of splashscreen. Applet one
>>> (which have error variation also), javaws one (forced by specification,
>>> which also forces title,vendor and homepage with description to be
>>> shown),
>>
>> This is not quite true. I know what the "specification" says, but other
>> implementations do not follow this particular requirement.
> 
> I have no problems to ignore this requirement, but I consider it quite
> reasonable (it shows to user what he is trying to download, and if it
> somehow similar what he is expecting to download)

Oh, some sort of indication is nice, but again, are you sure the splash
screen is the right place for this? For jnlp, we do show the urls in the
download indicators.

>>> and java's -spalsh which is showing image until control is
>>> forwarded to netx.
>>
>> Let's not worry about this one ;)
> 
> But I really like it! It cost us nothing, and when eg initial jnlp is
> downloaded very slowly it save a lot! Also for different tasks it is
> nice to see that IcedTea-web is running! (That "something is happening"
> by different way then to watch the LEDs on computer;) )

Hm..the idea is nice, but I am not a huge fan of it yet. Can we discuss
this separately? I think there are a few things that we can do here to
make it even better.

>>> The factory method have parameter whether to provide splash for javaws
>>> (it is showing also information from information element (vendor,
>>> description,homepage,title..) - specification commanding)
>>
>> Out of curiosity, why did you decide to implement InformationElement? Is
>> InformationDesc not sufficient?
> 
> As a keeper of information is absolutely enough. Think about it as of
> some kind of wrapper which is doing for spalsh specific operations which
> deserved to be implemented elsewhere - in this case i18n and searching
> for best matches. (And handling nulls :) )

I am not fully convinced yet, but I don't particularly see how this
could be bad (other than duplication more-or-less trivial stuff). So, okay.

>>> The showing of error screen is very successfull, but one case I was
>>> unable to catch - when connection with server dies in middle of resource
>>> transfer and TODO - I have forgot to test signatures and stuff around:-/
>>> - but this should be handled n netx-way too.
>>
>> Unit tests to check this, please!
> 
> Especially for signed jars very hard to do. But I will definitely kept
> in mind unit coverage. And will at least try the behaviour.

I said unit tests, not reproducers! ;)

Whether the jars are signed or not shouldn't affect unit tests at all.

>>> I'm showing also version on splash (and forwarding it to error mesage)
>>> (ok?) - Looks to me like good idea....
>>
>> Not sure if showing version on the splash itself is a great idea,
> Lets vote O:) Now it is 1:1 ;)
>> but I
>> agree it is good to show it in the error.
> It is.
>>

TBH, this doesn't seem like a big deal. If you want it, sure. But please
do consider if it will be helpful for the average user. There is no
point presenting them with extra information if that information is
meaningless to them.

>>> _bonuses_
>>> I have added recognition of two environment variables -
>>> ICEDTEA_WEB_PLUGIN_SPLASH and ICEDTEA_WEB_SPLASH. Those variables alow
>>> user to change splahs screen before lunchtime and - what is real reason
>>> for those two variables - to disable splashscreen at all.
>>
>> The only environment variable that IcedTea-Web currently uses is
>> ICEDTEAPLUGIN_DEBUG. It would be great if all the names were (somehow)
>> consistent.
> 
> yap!
> 
> what do you thing of:
> ICEDTEAPLUGIN_DEBUG
> ICEDTEAPLUGIN_SPLASH
> ICEDTEAWEB_SPLASH

So the first two are for the plugin, and the last one for netx? I feel
netx is not being given the same status as the plugin ;)

Cheers,
Omair