[icedtea-web] RFC: PR857: Don't set look and feel multiple times

[Jiri Vanek]

On 03/03/2014 06:42 PM, Omair Majid wrote:
> * Jiri Vanek <jvanek at redhat.com> [2014-03-03 02:51]:
>> On 03/01/2014 12:07 AM, Omair Majid wrote:
>>> Hi,
>>>
>>> The attached patch is a fix for PR857. The idea is quite simple: make
>>> sure UIManager.setLookAndFeel() is called at most once (and ideally
>>> exactly once) before any UI is shown.
>>>
>>> There are a few code paths that can cause UIManager.setLookAndFell to be
>>> invoked multiple times:
>>>
>>> - AboutDialog (only on entry from a class other than Boot)
>>> - SecurityDialog
>>> - PolicyEditor (only on entry from ControlPanel)
>>>
>>> CertificateViewer calls JNLPRuntime.initialize() which already calls
>>> UIManager.setLookAndFeel. So I have removed the direct call from
>>> CertificateViewer.
>>>
>>> Also, JNLPRuntime.initialize currently calls setLookAndFeel after
>>> displaying a JavaConsole. This is alos fixed.
>>>
>>> Thoughts?
>>>
>>
>> ouch, that an old one!
>>
>>
>> However - the original one - http://icedtea.classpath.org/bugzilla/attachment.cgi?id=640&action=diff
>> - (the confirmed one)  is quite different, why so?
>
> For a couple of reasons:
> - The reporter only tested one code path (the main plugin/javaws code
>    path)
> - I went through more code this time and noticed more subtle issues,
>    including cases where setting the look and feel is completely
>    redundant.
>
>> Also   " Don't set look and feel multiple times" do not sound proper
>> to me, it is still set two times.
>
> I don't follow. It is set exactly once in (a shared code path) in the
> case of plugins/javaws: in JNLPRuntime.initialize().
>
>> Can't there be only one place to do so?
>
> No, unless you want to have exactly one entry point for icedtea-web that then selects what to run:
> - javaws
> - plugin
> - about dialog
> - controlpanel
> - certificate viewer
> - policy editor
>
>> I would rather log the exception.
>>
>> I would ratehr log this exception in debug only (default) =?
>> -            OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
>> +            OutputController.getLogger().log(e);
>
> Fixed.


hmm. Then it is probably ok. If your conscience is clear, feel free to push.

But I still can see:
diff --git a/netx/net/sourceforge/jnlp/runtime/Boot.java b/netx/net/sourceforge/jnlp/runtime/Boot.java
--- a/netx/net/sourceforge/jnlp/runtime/Boot.java
+++ b/netx/net/sourceforge/jnlp/runtime/Boot.java
@@ -28,6 +28,8 @@
  import java.util.List;
  import java.util.Map;

+import javax.swing.UIManager;
+
  import net.sourceforge.jnlp.LaunchException;
  import net.sourceforge.jnlp.Launcher;
  import net.sourceforge.jnlp.ParserSettings;
@@ -161,6 +163,11 @@
              if (null != getOption("-headless")) {
                  JNLPRuntime.exit(0);
              } else {
+                try {
+                    UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
+                } catch (Exception e) {
+                    OutputController.getLogger().log("Unable to set system look and feel");
+                }
                  OutputController.getLogger().printOutLn(R("BLaunchAbout"));
                  AboutDialog.display();
                  return;
diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java 
b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
--- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
@@ -195,6 +195,12 @@
      public static void initialize(boolean isApplication) throws IllegalStateException {
          checkInitialized();

+        try {
+            UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
+        } catch (Exception e) {
+            OutputController.getLogger().log("Unable to set system look and feel");
+        }
+
          if (JavaConsole.canShowOnStartup(isApplication)) {
              JavaConsole.getConsole().showConsoleLater();
          }


It looks like duplication. Well it nitpicking to wont it unify somehow....Feel free to act as you 
wish. It probably just my personal issue that it looks nasty to me :)
However - It really can not be set before for both in common??


Sorry for nitpicking.

J.