> java.c: L427 it would be helpful to add a comment to explain
> the case where appClass is different than mainClass.
> Probably the comment above L425 should be updated to
> reflect the support for JavaFX
Done.
> L428-430: is this fallback needed? Would it be better
> if LauncherHelper.getApplicationClass() always returns
> a non-null class if the mainClass has been loaded successfully.
> Looks like this is the case in your implementation.
Good point, the helper would have aborted by that point. How about I change to
NULL_CHECK(appClass) just for safety's sake?
> LauncherHelper.java
> L746 missing space between 'if' and '('
> The javadoc for the checkAndLoadMain method would need to be
> updated
Done, along with a couple more that Kumar found.
> The change looks okay to me and I can't spot anything wrong there.
>
> L496-517 somewhat duplicates the logic added for FX in the
> getMainClassFromJar method. Have you considered some refactoring
> work you could do to simplify the fix since I think once you get
> the classname of the entry point (either from a JAR or command-line
> and with and without the static void main method), the logic is
> essentially the same. To elaborate, I see that FXHelper.launchName
> L707-725 is needed mainly to give a useful error message. When
> you find the classname of the entry point, perhaps you can load
> the class and catch any linkage error and determine if it's caused
> by the absence of FX runtime and output an appropriate error.
> If the main class is successfully loaded, then proceed with
> L496-517 (or something like that). Just an idea you can explore.
Yes, I do feel that especially in the -jar case there is some repetition. The
trouble is the ambiguity of ClassNotFoundException.
I'll poke at it and see what I can come up with.
> FXLauncherTest.java - very good test that covers many test cases.
> Do you plan to add the classpath case (i.e. not from a
> jar file)?
I hadn't, but if it's worthwhile then we could certainly add a test to do so.
Thoughts on this Steve?
-DrD-
