Replace native splash with Java implementation #53

stelfrich · 2018-04-09T08:51:14Z

This removes the native implementation that was hard to maintain in the past and wasn't working properly in some cases. In addition, changes to how libsplashscreen is linked starting with Java 9 has made using libjvm basically impossible.

The new Java implementation offers a progress bar during startup which is currently static. Future versions could improve on this and make the startup even more verbose s.t. we can get more information on where it grinds to a halt / an issue appears.

Thanks for working on this @ctrueden!

This closes #50 and closes #40.

This removes the native implementation that was hard to maintain in the past and wasn't working properly in some cases. In addition, changes to how libsplashscreen is linked starting with Java 9 has made using libjvm basically impossible. The new Java implementation offers a progress bar during startup which is currently static. Future versions could improve on this and make the startup even more verbose s.t. we can get more information on where it grinds to a halt / an issue appears.

ctrueden

Thanks so much for finalizing this, @stelfrich! Requested some minor changes.

Did you do much testing of non-headless CLI scenarios, to ensure they still don't splash?

ctrueden · 2018-04-09T16:04:45Z

pom.xml

@@ -129,7 +129,7 @@ Wisconsin-Madison, Broad Institute of MIT and Harvard, and Max Planck
 Institute of Molecular Cell Biology and Genetics.</license.copyrightOwners>
 		<license.projectName>ImageJ software for multidimensional image processing and analysis.</license.projectName>

-		<scijava.jvm.version>1.6</scijava.jvm.version>
+		<scijava.jvm.version>1.8</scijava.jvm.version>


Why? (If we do need this: we can just remove this line, rather than changing it, since 1.8 is the default these days.)
Edit: Oh, it was for the lambda below. I think we should change the code not to use a lambda, and instead be 1.6-compliant. Sorry about that, but it would keep the door open for a smarter launcher that complains when the version of Java is too old, rather than simply exploding.

ctrueden · 2018-04-09T16:06:45Z

src/main/java/net/imagej/launcher/SplashScreen.java

+		if (Boolean.getBoolean("java.awt.headless")) return;
+		final JWindow window = new JWindow();
+		splashWindow = window; // Save a non-AWT reference to the window.
+		final File logoFile = new File(new File(System.getProperty("imagej.dir")), LOGO_PATH);


Can we be a bit smarter about the path here? If imagej.dir is not set, would be nice to fall back to new File(".") or some such.

ctrueden · 2018-04-09T16:07:32Z

src/main/java/net/imagej/launcher/SplashScreen.java

+		splashWindow = null;
+	}
+
+	public static void main(final String[] args) throws Exception {


This main method was just for testing. Could you please remove it?

ctrueden · 2018-04-09T16:10:06Z

src/main/java/net/imagej/launcher/ClassLauncher.java

@@ -74,6 +74,9 @@
 	 */
 	public static void main(final String[] arguments) {
 		originalArguments = arguments;
+		if (Boolean.getBoolean("imagej.splash") || System.getProperty("imagej.splash") == null) {
+			SplashScreen.show();


This might be paranoid, but we may want to wrap the SplashScreen.show() with a try { ... } catch (Throwable t) { t.printStackTrace(); } because we really don't want any bugs in the splash screen to prevent ImageJ from starting up.

Issues with the splash should never stop ImageJ from starting.

stelfrich · 2018-04-10T07:32:58Z

Thanks for your feedback @ctrueden!

Did you do much testing of non-headless CLI scenarios, to ensure they still don't splash?

I thought I had and then went back to do some more testing only to find out that there was an issue with the --no-splash option. The behavior is now that -Dimagej.splash=true has to be set in order for the splash screen to show up. If it's set to false or is not set at all, the splash screen will not be shown. Also, the --headless option overrides the setting on the launcher level, plus we have an additional check in place.

I have tested

./ImageJ-linux64 → splash
./ImageJ-linux64 --no-splash → no splash
./ImageJ-linux64 --system --no-splash → no splash
./ImageJ-linux64 --headless → no-splash
./ImageJ-macosx → splash
./ImageJ-macosx --no-splash → no splash
./ImageJ-macosx --system --no-splash → no splash
./ImageJ-macosx --headless → no-splash
via SSH (no xvfb): - ./ImageJ-linux64 → no splash

If you are fine with the changes, I'll squash them into one commit (they don't seem educational) and merge to master.

ctrueden · 2018-04-13T02:47:07Z

What about the ./ImageJ-macosx --update command line tool?

stelfrich · 2018-04-25T13:46:28Z

./ImageJ-macos --update → no splash, but expected output
./ImageJ-macos --bsh → no splash, but interactive shell
./ImageJ-macos --javadoc → no splash, but expected error
./ImageJ-macos --javap → no splash, but expected error
What would we expect for ./ImageJ-macos --ij1, @ctrueden? Currently, the new splash screen is shown instead of the old one.

ctrueden · 2018-10-02T15:28:35Z

@stelfrich Sorry for dropping the ball on this. Any reasons not to merge this? The worst-case scenario would be that people's ImageJs don't launch anymore... but I think there is little danger of that here, no? If the launcher "over-splashes" (i.e. splashes in scenarios where it shouldn't), we can simply push out additional fixes. Right?

stelfrich · 2018-10-02T15:37:16Z

If the launcher "over-splashes" (i.e. splashes in scenarios where it shouldn't), we can simply push out additional fixes. Right?

Correct. Let's get this merged!

stelfrich force-pushed the splash-stelfrich branch from 2090b61 to 1d71f9b Compare April 9, 2018 08:52

This was referenced Apr 9, 2018

Support custom logo.png for update sites #54

Open

Support progress bar updates #55

Open

ctrueden requested changes Apr 9, 2018

View reviewed changes

stelfrich added 5 commits April 10, 2018 08:07

Remove lambda expression

2c87185

Remove main()

997f70d

Fetch exceptions from SplashScreen.show()

982f1dd

Issues with the splash should never stop ImageJ from starting.

Add fallback if -Dimagej.dir is not set

5f5603c

Don't splash if -Dimagej.splash is not set

fb1f044

stelfrich mentioned this pull request Oct 2, 2018

Investigate missing jvm.dll with Java10+ on Windows #58

Closed

ctrueden merged commit 271fb19 into master Oct 2, 2018

ctrueden deleted the splash-stelfrich branch October 2, 2018 17:24

ctrueden mentioned this pull request Nov 21, 2018

Retire fiji executable launchers #61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace native splash with Java implementation #53

Replace native splash with Java implementation #53

stelfrich commented Apr 9, 2018 •

edited

Loading

ctrueden left a comment

ctrueden Apr 9, 2018

ctrueden Apr 9, 2018

ctrueden Apr 9, 2018

ctrueden Apr 9, 2018

stelfrich commented Apr 10, 2018

ctrueden commented Apr 13, 2018

stelfrich commented Apr 25, 2018 •

edited

Loading

ctrueden commented Oct 2, 2018

stelfrich commented Oct 2, 2018

Replace native splash with Java implementation #53

Replace native splash with Java implementation #53

Conversation

stelfrich commented Apr 9, 2018 • edited Loading

ctrueden left a comment

Choose a reason for hiding this comment

ctrueden Apr 9, 2018

Choose a reason for hiding this comment

ctrueden Apr 9, 2018

Choose a reason for hiding this comment

ctrueden Apr 9, 2018

Choose a reason for hiding this comment

ctrueden Apr 9, 2018

Choose a reason for hiding this comment

stelfrich commented Apr 10, 2018

ctrueden commented Apr 13, 2018

stelfrich commented Apr 25, 2018 • edited Loading

ctrueden commented Oct 2, 2018

stelfrich commented Oct 2, 2018

stelfrich commented Apr 9, 2018 •

edited

Loading

stelfrich commented Apr 25, 2018 •

edited

Loading