Icedtea-web splashscreen implementation
Omair Majid
omajid at redhat.com
Wed Aug 1 12:30:21 PDT 2012
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:
>
>>> >The only "not fixed" is separation of
>>> net.sourceforge.jnlp.LaunchHandler
>>> >for each applet. I have trued to separate it somehow cleanly and
>>> >harmlessly but without success. So I would like to proceed without this
>>> >modification (especially for 1.3) and try to figure out more for head.
>> Hm.. I would have liked to see that.
>>
>
> So do I. But I really would like to separate this. I promise that I will
> look at this soon (although
> not imminently)
Fair enough.
>>> >+
>>> >+ public void paint(Graphics g);
>>> >+
>>> >+ public void paintComponent(Graphics g);
>>> >+
>>> >+ public void update(Graphics g);
>> I don't think paint/paintComponent/update to be exposed as public. They
>> are implementation details, right?
>>
>
> I have removed really redundant paintComponent and update, but kept
> paint. Splash is graphical
> object and have to be able to paint itself.
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.
> 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?
> I have also renamed those *12 clases to 2012 classes. My intention was
> to name it differently then
> "Default*" for case that in future new splashcreen will be implemented.
What if you implement a better splash screen this year ;)
> I'm not fan of adapters, but as you wish :)
This is more or less completely off-topic, but why?
>>
>>> >+ @Override
>>> >+ public void paintComponent(Graphics g) {
>>> >+ paint(g);
>>> >+ }
>>> >+
>>> >+ @Override
>>> >+ public void paint(Graphics g) {
>>> >+ painter.paint(g);
>>
>> I think just paintComponent should be overridden. Swing's implementation
>> of paint will call paintComponent at the right time.
>
> Disagree here. paint and paintcomponet are two different things called
> at different times (although
> (and you are right) for swing the only important one is paintComponent).
> State that both paint* are
> painting the same is just lucky coincidence and unless you insists, i
> would like to kept both
> methods filled.
I am not sure if that's a good idea. paint(Graphics) will invoke
paintComponent(Graphics) at the right time (and in the right order). By
overriding paint(Graphics) and not calling super.paint(Graphics), you
may cause issues later on (if you decide to replace a jpanel with
something else, for example).
>>
>>> >+ //waving of water and position of shhadowed WEB
>>> >+ private int level2 = 0;
>>> >+ private int l2inc = 15; //how quickly is greyed web moving
>> Likewise ;)
>>
>>> >+ //inidivdual sizes, all converging to ZERO!!
>>> >+ private final int WEB_TOP_ALIGMENT = 324;
>>> >+ private final int WEB_LEFT_ALIGMENT = 84;
>> How did you drive these numbers.
>
> :))) I was trying, measuring and testing until the splash looked well.
> (and was zooming correctly)
Okay, so we should somehow clarify that in the file itself. A comment
saying how these values were "measured" would be good.
>>
>>> >+ public final void startAnimationThreads() {
>> ...
>>> >+ };
>>> >+ tt.setDaemon(true);
>>> >+ return tt;
>>> >+ }
>> My first instic would be to have the animator thread in the controller.
>> The controller would signal when it's time for the view to update, and
> I would rather those threads encapsualted inside painter. Look at it
> like to MVC micro-environment;)
MVC micro-environment? So not mvc, then? ;) Please consider not mixing
multiple responsibilities into the view class. It should not have timing
logic in it.
>> the view would repaint itself. What shuts this thread down?
>
> Two stops by themselves, and the last (of main animation) is stopped by
> stopAnimation method when
> spalsh is about to be removed or repalced.
Please do not call setDaemon, then. It strongly implies that the threads
do not shut down.
>> Could you clarify this bit? Is this code copied from NatCubic?
>
> The only main computation is. As there is no licence and I have spoken
> (and got approve) with author
> it should be ok.
Okay, the comment was not very clear. How about something like this:
"""
This class is part of the NatCubic implementation (<url to natcubic>)
which does not have a license. The author (<Author name>) has agreed to
license this under GPL+Classpath (<link to the author saying yes>). The
NatCubic implementation was inspired by http://www.cse.unsw.edu.au/~lambert/
"""
Also, please fix the copyrights on the file. The copyrights are owned by
the original author. If you have modified it, then you need to add a Red
Hat copyright too. Please do this for all the NatCubic files.
>>
>>> >diff -r 01544fb82384
>>> netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ErrorPainter.java
>>>
>>> >--- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> >+++
>>> b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ErrorPainter.java
>>> Mon Jul 23 14:52:44 2012 +0200
>>> >+ private Thread getErrorScreamThread() {
>>> >+ // Create a new thread to screem error when failure
>> I am not sure I follow. Could you explain the purpose?
>
> When error occurs, big red ERROR fly through splash, but just for
> momnet. And then it is here no
> longer. This is what I have called ErrorScream:) During it fly the
> splash interpolate from normal
> state to greyed one with button to show stacktrace.
Sorry if this is nitpicking, but I think ErrorScream should be renamed.
The scream-bit is kind of misleading (it doesn't play any sound). How
about calling it ErrorIndicator?
>>> >diff -r 01544fb82384
>>> netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ImageFontCutter.java
>>>
>>> >--- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> >+++
>>> b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ImageFontCutter.java
>>> Mon Jul 23 14:52:44 2012 +0200
>>> >+public class ImageFontCutter {
>> From what I can tell, this is supposed to draw strings. Why the font
>> cutter name? Maybe something like TextRenderer might be more
>> appropriate? With a method named render:
>> render (Image image, String text, Font font) { .... }
>
> Actually not - it is really taking image and cut from this image shape
> in shapes of letters.
Okay. I don't know how this class is supposed to be used. You say it's
for getting the text outline, but the cutTo method (going by the class
name, it must by the focus of this class) returns void. In fact, by
calling setTextOutline, I can assign the outline.
>>
>>> >+ @Override
>>> >+ public String toXml() {
>>> >+ if (kind==null){
>>> >+ return super.toXml();
>>> >+ }
>>> >+ return "<"+type+" kind=\""+kind+"\">"+value+"</"+type+">";
>>> >+ }
>> Hm.. what's this for? Do we ever output anything in XML?
>
> 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?
>> 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.
> 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?
> + public InformationElement getInformationContent();
I guess you wanted to rename this to getInfomrationElement().
> + /** 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?
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.
> + /**
> + * 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?
> + */
> + public void startAnimation();
> +
> + /**
> + * Stops the animation
> + */
> + public void stopAnimation();
This comment doesnt help. Please remove it :)
> + 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.
> + public void setSplashReason(SplashReason splashReason);
> +
> + SplashReason getSplashReason();
public, please.
> + String getVersion();
Will any callers need to read this after setting it?
> + /**
> + * 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".
> + /**
> + * 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?
> + 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?
> +// 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.
> + if (DEFAULT.equals(pluginSplashEnvironmetVar)) {
Do we really need to use environment variables for this?
> + //if (sp==null) sp=new DummySplashScreen(width, height,splashReason);
> + //if (sp==null) sp=new CircleSplashScreen(width, height,splashReason);
Please remove this too.
> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/BasePainter.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/BasePainter.java Wed Aug 01 13:25:15 2012 +0200
> + private Thread getMoovingTextThread() {
Moving
> + Thread tt = new Thread() {
> +
> + @Override
> + public void run() {
> + while (master.isAnimationRunning()) {
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> + @Override
> + public void run() {
> + animationsPosition += greyTextIncrment;
> + if (animationsPosition > 10000) {
> + animationsPosition = 1;
> + }
Hey, magic numbers!
> + // Force repaint
> + master.repaint();
> + }
> + });
> + Thread.sleep(150);
> + } catch (Exception e) {
> + e.printStackTrace();
> + }
> + }
> + }
> + };
> + tt.setDaemon(true);
> + return tt;
> + }
> +
> + private Thread getWatterLevelThread() {
getWaterLevelThread.
> + // Create a new thread to increment spinFactor and repaint
> + Thread t = new Thread() {
> +
> + @Override
> + public void run() {
> + while (master.isAnimationRunning()) {
> + if (waterLevel > 120) {
> + break;
> + }
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + @Override
> + public void run() {
> + waterLevel += 2;
> + master.repaint();
> + }
> + });
> + Thread.sleep((waterLevel / 4) * 30);
> + } catch (Exception e) {
> + e.printStackTrace();
> + }
More magic numbers :(
> + }
> + }
> + };
> + t.setDaemon(true);
> + return t;
> + }
> +
> + private String getAlternativeProduktName() {
getAlternativeProductName()
But I would prefer if this method was just getProductName() and would
return the entire name.
> + 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?
> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/ErrorPainter.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/ErrorPainter.java Wed Aug 01 13:25:15 2012 +0200
> +public final class ErrorPainter extends BasePainter {
> +
> + //colors
> + private static final Color teaDeadColor = Color.darkGray;
> + private static final Color backgroundDeadColor = Color.gray;
> + private static final Color teaLeafsStalksDeadColor = new Color(100, 100, 100);
> + private static final Color pluginDeadColor = Color.darkGray;
> + private static final Color waterDeadColor = Color.darkGray;
> + private static final Color plainTextDeadColor = Color.white;
> + private static final String errorMessageKey = "SPLASHerror";
> + private static final String errorScreamMessageKey = "SPLASH_ERROR";
> + private static final Color errorScreamColor = Color.red;
Please not "scream".
Also, please use UPPER_CASE for constants.
> + //for clicking ot error message
> + private Point errorCorner = null;
> + private boolean errorIsScreamed = false;
> + private int errorScreamedPercentage = 100;
> +
> + public static Color interpolColor(double origSize, double currentSize, Color from, Color to) {
Should be named interpolateColor. Unless it is actually related to
Interpol ;)
> + private void interpoleColors(int origSize, int currentSize) {
Likewise.
> + private Thread getErrorScreamThread() {
> + // Create a new thread to screem error for failure
> + Thread t = new Thread() {
> +
> + @Override
> + public void run() {
> + try {
> + while (errorIsScreamed) {
> + errorScreamedPercentage -= 3;
> + interpoleColors(100, errorScreamedPercentage);
> + if (errorScreamedPercentage <= 5) {
> + errorIsScreamed = false;
> + setColors();
> + prerenderedStuff = null;
> + }
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + @Override
> + public void run() {
> + master.repaint();
> + }
> + });
> + Thread.sleep(75);
> + if (errorScreamedPercentage < 90 && errorScreamedPercentage > 80) {
> + clearCachedWaterTextImage();
> + canWave = false;
> + }
> + }
There are lots of magic constants here :(
> 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?
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.
> + public ImageFontCutter(Font f, String s, Color textOutline) {
s/textOutline/outlineColor/
> + /**
> + * @return the textOutline
> + */
The javadoc is meaning to the caller. textOutline is not public.
> + public Color getTextOutline() {
> + return textOutline;
> + }
> +
> + /**
> + * @param textOutline the textOutline to set
> + */
Same here; the javadoc is meaningless.
> + public void setTextOutline(Color textOutline) {
> + this.textOutline = textOutline;
> + }
> +
> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/parts/InfoItem.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/parts/InfoItem.java Wed Aug 01 13:25:15 2012 +0200
> +/**
> + *The optional kind="splash" attribute may be used in an icon element to indicate that the image is to be used as a "splash" screen during the launch of an application. If the JNLP file does not contain an icon element with kind="splash" attribute, Java Web Start will construct a splash screen using other items from the information Element.
> + *If the JNLP file does not contain any icon images, the splash image will consist of the application's title and vendor, as taken from the JNLP file.
> + *
> + * items not used inside
> + */
Maybe wrap this to 80 chars or so?
> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/parts/InformationElement.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/parts/InformationElement.java Wed Aug 01 13:25:15 2012 +0200
> +/**
> +http://docs.oracle.com/javase/6/docs/technotes/guides/javaws/developersguide/syntax.html
> + */
If you really want to use everything from the link why not use
InformationDesc?
> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialog.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialog.java Wed Aug 01 13:25:15 2012 +0200
> + static String getText(Throwable ex, List<String> l, String anotherInfo) {
> + StringBuilder s = new StringBuilder("<html><body>");
> + String info = "<p>"
> + + Translator.R(InfoItem.SPLASH + "mainL1") + " "
> + + " <a href=\""
> + + Translator.R(InfoItem.SPLASH + "url")
> + + "\">"
> + + Translator.R(InfoItem.SPLASH + "urlLooks") + "</a> "
> + + Translator.R(InfoItem.SPLASH + "mainL2")
> + + " </p> \n";
> + String t = "<p>"
> + + Translator.R(InfoItem.SPLASH + "mainL3")
> + + "</p> \n"
> + + info + formatListInfoList(l) + formatInfo(anotherInfo);
> + Object[] options = new String[2];
> + options[0] = Translator.R(InfoItem.SPLASH + "Close");
> + options[1] = Translator.R(InfoItem.SPLASH + "closeAndCopyShorter");
> + if (ex != null) {
> + t = "<p>"
> + + Translator.R(InfoItem.SPLASH + "mainL4")
> + + " </p>\n"
> + + info + formatListInfoList(l) + formatInfo(anotherInfo)
> + + "<p>"
> + + Translator.R(InfoItem.SPLASH + "exWas")
> + + " <br/>\n" + "<pre>" + getExceptionStackTraceAsString(ex) + "</pre>";
> +
> +
> + } else {
> + t += formatListInfoList(l);
> + }
> + s.append(t);
> + s.append("</body></html>");
> + return s.toString();
Just a heads up. This is probably not going to work out too well for
i18n. It may be other languages would read much better with reordered
text and building strings using + doesn't allow us to do that. Can you,
perhaps, use a single string for one message?
Thanks,
Omair
More information about the distro-pkg-dev
mailing list