Icedtea-web splashscreen implementation

Jiri Vanek jvanek at redhat.com
Mon Aug 13 08:15:17 PDT 2012


On 08/07/2012 06:19 PM, Omair Majid wrote:
> 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.

Don't worry! Although it does not sounds like it, I'm very *VERY* happy for your careful and 
enlightening review! And I 100% agree with your argument above.
>
>> Anyway most of below faithfully fixed
>
> Thank you for your patience.
>

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

When I was writing this "argument" I have sensed this reply :)
Agree with you, made package private (although it is not the best for you As far as I can judge:( )
>>>> 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.

Thank you for this.. nearly forgot about it!

>
>> ok.  Renamed to FlyingError* - and you must confess you have not seen
>> animation;)
>
> I did see the 90MB (?) gif you linked to :)
>
My deepest apologise then!

>>>> I'm using it in tests to generate jnlps on the  fly.
>>>
...longsnip...
>
> I must have missed something. This is never executed, right? Do you
> think this code would be helpful later?

No You have not missed anything and you are right. I will add this swith if some new splash will 
come in future.
>
> If so, feel free to add the DummySplashScreen/CircleSplashScreen classes
> and make this entire code dependent on some conditional.

Will there be time for this?? I'm afraid not, but dummy developer's one would be maybe useful:) soon:(
>
>>>
>>>> +            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.

eh no... Is is just number. I made it package private.

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

Ugh, I have reworked it as you wished, but it really does look nasty. So I have commit it as it was. 
Sorry:(

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

Ok. Stuff pushed - to kept it "compact" I have added two and half  minor (I hope)  modification from 
integration:

http://icedtea.classpath.org/hg/icedtea-web/rev/a86af88a8ecd#l4.1 + relevant 
http://icedtea.classpath.org/hg/icedtea-web/rev/a86af88a8ecd#l2.1

and  - http://icedtea.classpath.org/hg/icedtea-web/rev/a86af88a8ecd#l5.1 (corrected by local people, 
so I hope nothing will be found here)


Also SplashUtils.java have been pushed as it it were already discuses.

To add this three changesets have saved me a lot of refactoring due to split, If you are not ok with 
them, please let me know immediately.
>
> Sorry for causing you so much trouble with the patch.

Do not apologise! I'm perfectly happy with your approach!

Attached is patch with integration "adapted for head"

Thank you very much!

J.
>
> Omair
>
> [1] http://en.wikipedia.org/wiki/You_ain%27t_gonna_need_it
> [2] http://en.wikipedia.org/wiki/Liskov_substitution_principle
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: splash_Integration_VII.diff
Type: text/x-patch
Size: 30074 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120813/4c53be70/splash_Integration_VII.diff 


More information about the distro-pkg-dev mailing list