Icedtea-web splashscreen implementation
Jiri Vanek
jvanek at redhat.com
Thu Nov 1 08:32:40 PDT 2012
On 10/29/2012 07:42 PM, Omair Majid wrote:
> Hi Jiri,
>
> Comments in-line below.
Thank you :)
But you are giving me hard times. It needs a lot of garbage-searching in deep corners of my nearly-forgotten memories to recall why I have done something.
I hope You will like this new patch more!
>
> On 10/24/2012 05:53 AM, Jiri Vanek wrote:
>> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>> --- a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java Tue Oct 23 09:56:26 2012 +0200
>> +++ b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java Wed Oct 24 11:46:08 2012 +0200
>> @@ -80,10 +80,13 @@
>> }
>>
>> private void closeSplashScreen() {
>> - synchronized(mutex) {
>> + synchronized (mutex) {
>> if (splashScreen != null) {
>> if (splashScreen.isSplashScreenValid()) {
>> splashScreen.setVisible(false);
>> + if (splashScreen.isCustomSplashscreen()) {
>> + splashScreen.stopAnimation();
>> + }
>
> Perhaps splashScreen.stopAnimation() can be called without the
> splashScreen.isCustomSplashscreen check? stopAnimation() already does
> this check.
Sure, sorry.
>
>> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/JNLPSplashScreen.java
>> --- a/netx/net/sourceforge/jnlp/JNLPSplashScreen.java Tue Oct 23 09:56:26 2012 +0200
>> +++ b/netx/net/sourceforge/jnlp/JNLPSplashScreen.java Wed Oct 24 11:46:08 2012 +0200
>
>> + //Toolkit.getDefaultToolkit().getScreenSize(); works always, but center
>> + //only to middle of all monitors.
>> + //Below code works on 1.4 and higher, and center to middle of primary monmtor
>
> This comment, just by itself, does not make too much sense (I guess it
> requires the reader to know about the previous implementation). Perhaps
> you can rephrase it?
>
> // Centering to middle of Toolkit.getDefaultToolkit().getScreenSize()
> // centers to the middle of all monitors. Let's center to the middle
> // of the primary monitor instead.
> // TODO center on the 'current' monitor to meet user expectation
As you wish:)
>
>
>> + setLocationRelativeTo(null);
>> }
>>
>> @Override
>> public void paint(Graphics g) {
>> if (splashImage == null) {
>> + super.paint(g);
>
> Nice catch!
>
>> +
>> + boolean isCustomSplashscreen() {
>> + return (componetSplash!=null);
>> + }
>> +
>> + void stopAnimation() {
>> + if (isCustomSplashscreen()) componetSplash.stopAnimation();
>> + }
>
> Please consider making these methods public or private.
It was intentional to keep them package-private, but there is no harm to make them public. Done.
>
>> }
>> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/Launcher.java
>> --- a/netx/net/sourceforge/jnlp/Launcher.java Tue Oct 23 09:56:26 2012 +0200
>> +++ b/netx/net/sourceforge/jnlp/Launcher.java Wed Oct 24 11:46:08 2012 +0200
>
>> protected ApplicationInstance launchApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
>
>> + if (JNLPRuntime.getForksAllowed()&& file.needsNewVM()) {
>> + if (!JNLPRuntime.isHeadless()) {
>> + SplashScreen sp = SplashScreen.getSplashScreen();
>> + if (sp != null) {
>> + sp.close();
>> + }
>> + }
>
> This is really puzzling. Forks are never allowed for applets, so the
> first if statement should never evaluate to true. But if we could start
> new VMs for applets, we would need to prevent applets from running in
> this VM. We need to return from this method to avoid executing further
> code, but we also need to notify the plugin that another VM now handles
> the plugin.
Really? Not even when launched from jnlfile even with xmx?? I thin there is no reason not to allow them...
However I have followed your advice. But I really think that applets launched via jnlp should be able to fork...
>
> I think it's just simpler to remove this code. Maybe add this instead?
>
> if (JNLPRuntime.getForksAllowed()) {
> throw new AssertionError("applets are not allowed to fork");
> }
>
>> + }
>> + if (handler != null) {
>> + handler.launchInitialized(file);
>> + }
>
> Please don't add null checks. Why is the handler null? Maybe it should
> be set to a no-op handler instance (in the constructor of this class) if
> it's not otherwise set?
hmm.. I see your point. But for this I have to had some reason... I feel better with this null check. I have kept this in.
>
>> - protected AppletInstance createApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
>> + protected synchronized AppletInstance createApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
>
> Okay, this "synchronized" keyword here looks rather out of place. If
> synchronization is really needed, I don't think we should be adding it
> to this class.
Well here we have an disagreeing. I have played with those two newly synchronised methods again.
When two (or even more) applets are on same page, launched together, without both method synchronised the splashscrens are initiated wiredly:(
After those tests I have kept those new synchronisation in code.
What is actually your reason not to add them?
>
>> + // appletInstance is needed by ServiceManager when looking up
>> + // services. This could potentially be done in applet constructor
>> + // so initialize appletInstance before creating applet.
>
> Could you leave this change out of the patch? Let's not mix in multiple
> fixes/changes.
>
Done. Although with heavy heart because I have no Idea why I it ever write inside...
But I suppose you are right with removing it:)
> Also, I am not sure if applets are allowed to use the ServiceManager
> methods until init() is invoked. They are not allowed to call applet
> methods, for example:
> http://docs.oracle.com/javase/7/docs/api/java/applet/Applet.html#Applet()
>
>> + AppletInstance appletInstance = null;
>> + ThreadGroup group = Thread.currentThread().getThreadGroup();
>> try {
>> JNLPClassLoader loader = JNLPClassLoader.getInstance(file, updatePolicy);
>>
>> if (enableCodeBase) {
>> loader.enableCodeBase();
>> } else if (file.getResources().getJARs().length == 0) {
>> - throw new ClassNotFoundException("Can't do a codebase look up and there are no jars. Failing sooner rather than later");
>> + Exception ex = new ClassNotFoundException("Can't do a codebase look up and there are no jars. Failing sooner rather than later");
>> + try {
>> + SplashUtils.showError(ex, new AppletEnvironment(file, new AppletInstance(file, group, new ClassLoader() {
>> + }, null)));
>> + } catch (Exception exx) {
>> + if (JNLPRuntime.isDebug()) {
>> + exx.printStackTrace();
>> + }
>> + }
>
> Would it be possible to use launchError to handle this message rather
> than using SplashUtils directly?
Actually not:) But I have removed whole spalsh related stuff from this catch block, because exception is catch later and in this place properly shown by ErrorSpalsh. Thank you for hint.
>
>> private LaunchException launchError(LaunchException ex) {
>> + return launchError(ex, null);
>> + }
>> +
>> + private LaunchException launchError(LaunchException ex, AppletInstance applet) {
>> + if (applet != null) {
>> + SplashUtils.showErrorCaught(ex, applet);
>> + }
>
> RFE: I would like to see (in some future patch) a applet-specific
> handler that can be used to avoid needing a explicit call to SplashUtils.
>
>
>> + public void setAppletViewerFrame(SplashController framePanel) {
>> + appletViewerFrame=framePanel;
>> + }
>> +
>> + @Override
>> + public void removeSplash() {
>> + appletViewerFrame.removeSplash();
>> + }
>> +
>> + @Override
>> + public void replaceSplash(SplashPanel r) {
>> + appletViewerFrame.replaceSplash(r);
>> + }
>> +
>> + @Override
>> + public int getSplashWidth() {
>> + return appletViewerFrame.getSplashWidth();
>> + }
>> +
>> + @Override
>> + public int getSplashHeigth() {
>> + return appletViewerFrame.getSplashHeigth();
>> + }
>
> Interesting that all these methods are manipulating the
> appletViewerFrame. Looks to me like they belong in PluginAppletViewer
> class. Or maybe the instance variable appletViewerFrame should be
> renamed to splashController. But it still seems odd that these methods
> are in this class if all they are doing is passing the calls along to
> the SplashController.
Yes. This class is just exposing inside-SplashControler. I had the name toremeber that spalshcotroler inside IS AppletViewer.
However - Renamed.
>
>> diff -r c52fcd37dbd8 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Tue Oct 23 09:56:26 2012 +0200
>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Wed Oct 24 11:46:08 2012 +0200
>
> I would like someone more knowledgeable than me to review this piece of
> code too. There are lots of changes with implications that I am unaware of.
:(
>
>> @@ -145,7 +149,12 @@
>> @Override public void run() {
>> panel.createNewAppContext();
>> // create the frame.
>> - PluginAppletViewer.framePanel(identifier, handle, panel);
>> + int width = Integer.parseInt(atts.get("width"));
>> + int height = Integer.parseInt(atts.get("height"));
>> +
>> + PluginDebug.debug("X and Y are: " + width + " " + height);
>
> The width/height are being obtained twice now. Is the second invocation
> needed?
Checked and fixed. Although I have felt better with previous satte, it should be ok as it is in this new one (extracted get("width/height"))
>
>> - public static void framePanel(int identifier, long handle, NetxPanel panel) {
>> + public static synchronized PluginAppletViewer framePanel(int identifier,long handle, int width, int height, NetxPanel panel) {
>
> Maybe this method should be renamed to createPanel now? It's not framing
> the panel anymore.
>
> Also, why the 'synchronized' keyword. What requires synchronization?
>
>> + final AppletPanel fPanel = panel;
>> + try {
>> + SwingUtilities.invokeAndWait(new Runnable() {
>> +
>> + public void run() {
>> + add("Center", fPanel);
>> + fPanel.setVisible(false);
>> +
>> + splashPanel = SplashUtils.getSplashScreen(fPanel.getWidth(), fPanel.getHeight());
>> + if (splashPanel != null) {
>> + splashPanel.startAnimation();
>> + PluginDebug.debug("Added splash " + splashPanel);
>> + add("Center", splashPanel.getSplashComponent());
>> + }
>> + pack();
>> + }
>> + });
>> + } catch (Exception e) {
>> + e.printStackTrace(); // Not much we can do other than print
>> + }
>
> This new code looks like it really should be in its own method.
Done.
>
>> + @Override
>> + public void removeSplash() {
>> +
>> + // Loading done. Remove splash screen.
>> + if (splashPanel == null) {
>> + return;
>> + }
>
> Is the comment relevant? At first I thought it was saying that
> splashPanel == null implies that loading is done. But going through the
> code, it looks like it just makes this method a no-op after first
> invocation.
No. Removed more this-one-like comments
>
>> + try {
>> + SwingUtilities.invokeAndWait(new Runnable() {
>> +
>> + public void run() {
>> + splashPanel.getSplashComponent().setVisible(false);
>> + splashPanel.stopAnimation();
>> + remove(splashPanel.getSplashComponent());
>> + splashPanel = null;
>> + // Re-add the applet
>> + remove(panel);
>> + add(panel);
>
> I am guessing the remove()/add() is need to notify the container of
> changes? Maybe the comment can say why that's being done instead of
> saying what's being done.
Done.
>
>> + case AppletPanel.APPLET_START: {
>> + String s="Error1 detected";
>> + PluginDebug.debug(s);
>> + if (src.status != AppletPanel.APPLET_INIT&& src.status != AppletPanel.APPLET_STOP) {
>> + SplashPanel sp=SplashUtils.getErrorSplashScreen(appletViewer.panel.getWidth(), appletViewer.panel.getHeight(), new Exception(s));
>> + appletViewer.replaceSplash(sp);
>> + }
>> +
>> + break;
>> + }
>
> Could you add a comment explaining how APPLET_START is an error?
Eeeerrr. no:) The error is only inside if block. Both error messages (above and below) rewritten.
>
>> + case AppletPanel.APPLET_ERROR: {
>> + String s="Error2 detected";
>> + PluginDebug.debug(s);
>
> The debug message doesn't explain anything. Could you add more information?
>
>> + System.err.println("Waiting for applet init");
>
> Sounds like this should be a PluginDebug.debug() message.
Srure.
>
Thank you again,
j.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: splash_Integration_X.diff
Type: text/x-patch
Size: 34249 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121101/1790e5f5/splash_Integration_X.diff
More information about the distro-pkg-dev
mailing list