Icedtea-web splashscreen implementation
Omair Majid
omajid at redhat.com
Tue Aug 7 09:19:22 PDT 2012
Hi Jiri,
On 08/07/2012 04:50 AM, Jiri Vanek wrote:
> On 08/01/2012 09:30 PM, Omair Majid wrote:
>
> Hi! You are an tormentor and I'm starting to bleed from my eyes ;)
Uh. Sorry for the lots of nits. Let me explain my point of view.
This patch is rather significant and adds lots of new code. Every line
of this new code will be executed every time the plugin (or javaws)
runs. It will have bugs and (possibly, but not very likely) security
vulnerabilities. Some developer (very likely someone other than you)
will have to update, modify, and improve things based on user feedback.
I have committed a number of patches to icedtea-web where I took the
simple/easy way to do fix something and made things much harder for the
future (*cough* JNLPClassLoader *cough*). If I am being pedantic, it's
because I want this code to stick around and be useful instead of
causing us headaches down the line.
> Anyway most of below faithfully fixed
Thank you for your patience.
>> Hi Jiri,
>>
>> Lots of nits below.
>>
>> On 08/01/2012 09:55 AM, Jiri Vanek wrote:
>>>
>>> on 07/31/2012 03:33 AM, Omair Majid wrote:
>>>
> ...snip...
>>
>> Oh, I agree. But should another class be able to tell it when to paint?
>> I see below that you want to make it a self-contained MVC (more on that
>> later), so external classes have no need for telling it when to paint
>> itself. The implementation of this interface needs to have paint
>> methods, but it need not be part of the interface.
>
> hmhm... I do not wont to get rid of this. I have renamed paint(g) to
> paintTo(g) to avoid confusion with classical paint but to still to be
> publicly paintable.
> Can you survive this?
Please don't misunderstand me. The next developer who finds this method
will be wondering when and how this method is supposed to be invoked. I
think this method is an implementation detail, so it should be left out
of the interface. If I am wrong, please document when and how this
method is supposed to be called and leave it in.
> (say no and it will be deleted next time - I'm out of arguments :)
>>
>>> Get percentage was kept, and other getters moved to package private for
>>> testing. Is it ok for you?
>>
>> Package-private sounds fine. Is getPercentage used by something?
>
> no - but can be useful when implementing DownloadServiceListener
YAGNI [1] :) I would like to cross that bridge when we get to it. Feel
free to leave it in.
>>> I'm not fan of adapters, but as you wish :)
>>
>> This is more or less completely off-topic, but why?
>>
> The aggregator - I dont like too O:) - can sometimes not be avoided.
> But aggregator which is dedicated to have just one member == adapter is
> considered be me as error in design. However after this example when it
> saves a lot of duplicated code it is forcing me to reconsider.
>
> and still code like
>
> void method(){
> variable.doTheMethod();
> }
>
> blah,looks nasty.
>
That's a valid point. But when designing classes, I like to keep the
Liskov substitution principle [2] in mind when deciding whether a object
should extend another object or contain it.
> ok. Renamed to FlyingError* - and you must confess you have not seen
> animation;)
I did see the 90MB (?) gif you linked to :)
>>> I'm using it in tests to generate jnlps on the fly.
>>
>> I admit I cant figure out a use case for this. What are you trying to do?
> I'm generating jnlps and then reading them and testing resoults of
> InformationElement - especially of descriptions and theirs kinds. It
> sounded to me like more typo-proof then write the descriptions again and
> again.
> I agree that this is little bit in bad file. Unless you find it usefull,
> I have moved it to inner class in unittests. to just-for-testing
> descendants.
Thanks; sounds like the right fix.
>>>> Hm... is this code running with full privileges? Can it access the
>>>> clipboard?
>>>
>>> it is funny, but this works both with signed and unsigned applets
>>> (because of package
>>> net.sourceforge.jnpl?).
>>
>> No, that wouldnt matter. I guess this code is always run without any
>> non-jdk and non-icedtea-web code on the stack.
>
> hmmm Can this be somehow harmful?
It should be perfectly safe (it may cause a SecurityException if it is
called with unprivileged code on the stack). It can only cause harm if
the JRE itself has security vulnerabilities.
>>> diff -r 01544fb82384
>>> netx/net/sourceforge/jnlp/splashscreen/SplashPanel.java
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++ b/netx/net/sourceforge/jnlp/splashscreen/SplashPanel.java Wed
>>> Aug 01 13:25:15 2012 +0200
>>> @@ -0,0 +1,124 @@
>>
>>> +public interface SplashPanel {
>>
>>> + /**
>>> + * javaws should provide content of <information> tag. Those
>>> informations hould be passed by this method
>>> + */
>>> + public void setInformationElement(InformationElement content);
>>
>> You can leave out the comment here. It isn't helping. Perhaps it should
>> be part of the InformationElement class?
> done
>>
>>> + public InformationElement getInformationContent();
>>
>> I guess you wanted to rename this to getInfomrationElement().
>
> sorry for not do in last round. Done now.
>
>>
>>> + /** Width of the plugin window */
>>> + public void setPluginWidth(int pluginWidth);
>>> +
>>> + /** Height of the plugin window */
>>> + public void setPluginHeight(int pluginHeight);
>>> +
>>> + /** Width of the plugin window */
>>> + public int getPluginWidth();
>>> +
>>> + /** Height of the plugin window */
>>> + public int getPluginHeight();
>>> +
>>> + public void adjustForSize();
>>
>> You said this was fixed, but I am not sure it is.
>>
>> First, why the 'plugin' in the names? Is getWidth/setWidth not
>> sufficient?
>>
> nope - getwidth/height can overwrite the ones with same name in swing.
> However i have renamed them to set/getSplashWidth/Height
>
>> Why is adjustForSize a separate method. Why not just have a single
>> method called setSize(int width, int height) that does the work of
>> setPluginWidth, setPluginHeight and adjustForSize. Please understand I
>> am only asking for this in the interface. The implementation is free to
>> have a separate adjustForSize method.
>
> This was of course my first idea. But then I came to cases when was
> better to setSize and then do something else and at the end recalcualte
> rest.
> I got inspired by pack/validate.
>
>>
>>> + /**
>>> + * Methods to start the animation in the splash panel.
>>> + *
>>> + * This method exits after starting a new thread to do the
>>> animation. It
>>> + * is synchronized to prevent multiple startAnimation threads
>>> from being created.
>>
>> This second paragraph sounds like implementation details leaking. Is
>> there a reason the caller has to know about this?
>
> Sure removed.
>>
>>> + */
>>> + public void startAnimation();
>>> +
>>> + /**
>>> + * Stops the animation
>>> + */
>>> + public void stopAnimation();
>>
>> This comment doesnt help. Please remove it :)
>>
> done
>
>>> + public void paint(Graphics g);
>>> +
>>> + /**
>>> + * Decide wather to show icedtea-web plugin or just icedtea-web
>>> + * @param splashReason
>>> + */
>>
>> Please remove this. This is the details about the SplashReason class. It
>> probably belongs in SplashReason.
> done
>>
>>> + public void setSplashReason(SplashReason splashReason);
>>> +
>>> + SplashReason getSplashReason();
>>
>> public, please.
> done
>
>>
>>> + String getVersion();
>>
>> Will any callers need to read this after setting it?
> hard to say. I'm used to have getter, unless there is strong reason to
> not have. But I agree with you approach of as thin interface as possible.
> To thin interface can however cause hard testablity or adding of
> necessary methods later.
>>
>>> + /**
>>> + * how mny percentage loaded is shown in progress bar (if any)
>>> + * @param done
>>> + */
>>> + public void setPercentage(int done);
>>
>> Could you add the expected values the comment the? Something like:
>> "@param done must be between 0 and 100, inclusive".
> done
>>
>>> + /**
>>> + * returns state of loading progress bar
>>> + * @return
>>> + */
>>> + public int getPercentage();
>>
>> I am still curious what object needs to read this. Maybe that object
>> needs to talk to the controller instead of this view.
>>
>>> diff -r 01544fb82384
>>> netx/net/sourceforge/jnlp/splashscreen/SplashUtils.java
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++ b/netx/net/sourceforge/jnlp/splashscreen/SplashUtils.java Wed
>>> Aug 01 13:25:15 2012 +0200
>>
>>> +public class SplashUtils {
>>
>>> + public static void showError(Throwable ex, AppletEnvironment ae) {
>>> + if (ae == null) {
>>> + return;
>>> + }
>>> + SplashController p = ((SplashController)
>>> (ae.getAppletFrame()));
>>
>> I dont like this cast. It's not obvious. Maybe you can add a method
>> called getSplashScreen to AppletEnvironment instead?
>
> done, true, it was really not obvious.
>
>>
>>> + static SplashPanel getSplashScreen(int width, int height,
>>> SplashUtils.SplashReason splashReason, Throwable loadingException,
>>> boolean isError) {
>>
>> Can't we figure out the value of isError based on loadingException being
>> null or not?
>
> No. I had this longer ago, but there was causes when no exception
> arrived. then I tought it is better to have some text with "no
> exception" rather then to sent some dummy exception.
>
> If you insists I can solve this by some custom exception which will just
> replace null-state and will be handled little bit differently in error
> dialogue.
>
>>
>>> +// if ("circle".equals(splashEnvironmetVar)) {
>>> +// sp = new CircleSplashScreen(width, height,
>>> splashReason);
>>> +// }
>>> +// if ("dummy".equals(splashEnvironmetVar)) {
>>> +// sp = new DummySplashScreen(width, height,
>>> splashReason);
>>> +// }
>>
>>> +// if ("circle".equals(pluginSplashEnvironmetVar)) {
>>> +// sp = new CircleSplashScreen(width, height,
>>> splashReason);
>>> +// }
>>> +// if ("dummy".equals(pluginSplashEnvironmetVar)) {
>>> +// sp = new DummySplashScreen(width, height,
>>> splashReason);
>>> +// }
>>
>> Please remove this.
> Done:( Although I have hard heart leaving it out :((
I must have missed something. This is never executed, right? Do you
think this code would be helpful later?
If so, feel free to add the DummySplashScreen/CircleSplashScreen classes
and make this entire code dependent on some conditional.
>>
>>> + if (DEFAULT.equals(pluginSplashEnvironmetVar)) {
>>
>> Do we really need to use environment variables for this?
> We have already discuses this in original thread and we agree on this
> approach. (???or not??)
Ah, right. I seem to recall something about that now :)
>>> + public int getWaterLevel() {
>>> + return waterLevel;
>>> + }
>>> +
>>> + public void setWaterLevel(int level) {
>>> + this.waterLevel = level;
>>> + }
>>> +
>>> + public int getAnimationsPosition() {
>>> + return animationsPosition;
>>> + }
>>
>> What do these numbers (level and position) represent?
>
> What they say - the rising water in letters and position of secondary
> animation (grey waiving text) :) -brm brm brm..animation? :D
What I meant was, if i call getAnimationPosition() and it returns 100,
what does that mean? Is it the percentage of the animation? Same thing
for the water level.
>>> diff -r 01544fb82384
>>> netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/ImageFontCutter.java
>>>
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/ImageFontCutter.java
>>> Wed Aug 01 13:25:15 2012 +0200
>>
>>> +public class ImageFontCutter {
>>
>> I still think the name is misleading. It renders text onto a graphics
>> object using a clipped image rather than a solid color, right? How about
>> renaming to TextOutlineRenderer?
>
> Ugh. As you wish, faithfully renamed.
>>
>> Looking at how TextWithWaterLevel uses this, it looks a bit muddled.
>> Though after staring at it for a while, it is starting to make a lot of
>> sense.
>>
>> My recommendation is to not extend this class in TextWithWaterLevel (or
>> any other place where this is used). Use composition rather than
>> inheritance.
> nnnnnnn:(( If it is recommendation - can you live with inheritance?
Yes, I can :)
But please do consider if a "has-a" relationship is more appropriate
than a "is-a".
Please go ahead and commit the "implementation" parts of the splash
screen (and any tests you feel like) to HEAD (but please do consider my
suggestions above for making improvements). I will try and review the
"integration" parts soonish.
Sorry for causing you so much trouble with the patch.
Omair
[1] http://en.wikipedia.org/wiki/You_ain%27t_gonna_need_it
[2] http://en.wikipedia.org/wiki/Liskov_substitution_principle
More information about the distro-pkg-dev
mailing list