Add module-info descriptors and ResourcesProvider

[jperedadnr]

Issue

Fixes #719

Progress

• Change must not contain extraneous whitespace
• License header year is updated, if required
• The PR name must follow the pre-defined format
• Verify the contributor has signed Gluon Individual Contributor License Agreement (CLA)

[Oliver-Loeffler]

That was a lot of work. I'll finish the review tonight.

[abhinayagarwal]

Tested on both vanilla maven app and bundled dmg.

[Oliver-Loeffler]

Technically this change is fine when running plain Maven based builds. Interestingly, this setup generates errors when using Eclipse and importing SceneBuilder as a Maven-Project. Here an error is created which prevents project compilation as there is a test class in kit wich has no package declaration. Not sure if this is Maven's fault or Eclipse fault, it does not work out of the box in Eclipse.

The error does not occur in Idea 2024.2.0.1, but indeed in Eclipse 2024-09 (2024-09, 4.33.0, Build id: 20240905-0614).

Furthermore, when attempting to run tests from within Eclipse, those may fail due to `java.lang.reflect.InaccessibleObjectException``. Such as:

java.lang.reflect.InaccessibleObjectException: Unable to make void javafx.fxml.FXMLLoader.setStaticLoad(boolean) accessible: module javafx.fxml does not "opens javafx.fxml" to module com.gluonhq.scenebuilder.kit

(in: kit\src\test\java\com\oracle\javafx\scenebuilder\kit\editor\job\RelocateSelectionJobTest.java)

I have played around with having a 3rd Maven module just called tests. It would only have one dependency to kit and it works for Maven, Eclipse and Idea.

As tests are different source trees, one could consider them (especially with modules in mind) as separate Maven module.
I have made positive experience with separating tests into a different sub-project in other projects. What do you think about this @jperedadnr?

Using a Maven module for tests, we could have (a) one place for test infrastructure as the JFXInitializer and we could tests modular vs. classpath usages.

There are also multiple locations where warnings are generated within Eclipse, about missing declaration of transitive dependencies in module-info. One example is:

As this is public API, this can be resolved in a way such as:

module com.gluonhq.scenebuilder.kit {
    requires transitive javafx.controls;
    requires javafx.fxml;
    requires javafx.swing;
    requires javafx.media;
    requires javafx.web;
    requires transitive javafx.graphics;
...}

This is important for kit but is also an issue for app, as app also has lots of public API.

[jperedadnr]

Thanks for your review, @Oliver-Loeffler

I've added the require transitive.

As for InaccessibleObjectException, I've added to the pom:

               <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-surefire-plugin</artifactId>
                    <version>3.0.0-M5</version>
                    <configuration>
                        <reuseForks>false</reuseForks>
                        <forkCount>1</forkCount>
 +                       <argLine>--add-opens=javafx.fxml/javafx.fxml=com.gluonhq.scenebuilder.kit</argLine>
                    </configuration>
                </plugin>

So I guess that you need the same if you run tests from within your IDE and not from the plugin?

As for the failing test on Eclipse, it kind of makes sense, under the modular world a class has to have a package name, so even if you move it to a third maven module "tests", the issue will remain the same unless, of course, we have non-modular-tests maven module (after all, we would be testing issues from any JavaFX project using FXML files, but not modules), and also modular-tests maven module.

I'm fine with moving tests to its own module(s) (in fact, I'm back working on "Components", after #241 ), but this should be done in a follow-up PR. So maybe we need to comment this test out (and related classes).

[Oliver-Loeffler]

@jperedadnr I've reviewed a 2nd time and identified some more packages and modules which require the transitive keyword. I've also found 2 missing exports. Furthermore there are 2 classes which need to be declared as public. After that, I think the PR is ready to by merged and we shall no longer see warnings related to module-info.java. I agree with your proposal - sorry for the 2nd run on this topic.

[jperedadnr]

Can you list those changes? I don't find those issues from my IDE.

[Oliver-Loeffler]

Can you list those changes? I don't find those issues from my IDE.

Changes in kit:

So following changes in kit / module-info.java:

    requires transitive javafx.fxml;
    requires transitive javafx.web;
    requires transitive static java.prefs;
    requires transitive static aether.api;

Classes to become public:

• com.oracle.javafx.scenebuilder.kit.skeleton.SkeletonContext

Changes in app:

Following changes in app / module-info.java

    requires transitive com.gluonhq.scenebuilder.kit;
    exports com.oracle.javafx.scenebuilder.app.preferences;
    exports com.oracle.javafx.scenebuilder.app.menubar;

Classes to become public:

• com.oracle.javafx.scenebuilder.app.ResourceController

Warnings in Eclipse IDE:

• kit

• app

I've added a review commend at the particular positions in the respective module-info files.

[jperedadnr]

I've added transitive to the javafx and java.pref modules in kit, and transitive to kit module in app.

I haven't added transitive to aether.api though, as that means that anyone consuming the kit will need to add the aether module, or :

[ERROR] Compilation failure
[ERROR] module not found: aether.api

What error do you get without this? Or is it just a warning?

Also I don't understand why the need of making public those two classes. For the kit.skeleton package, I see we export it (for instance, DocumentWindowController in app uses the SkeletonWindowController), however most of the classes in such package are private-package, like SkeletonContext, which seems fine, doesn't it?

But I see that public SkeletonConverter has public API with SkeletonContext, so that makes sense...

[jperedadnr]

Note about Warnings/Errors:
If I add to the maven compiler plugin:

<configuration>
                        <useIncrementalCompilation>false</useIncrementalCompilation>
                        <showWarnings>true</showWarnings>
                        <compilerArgs>
                            <arg>-Xlint:all</arg>
                            <arg>-Werror</arg>
                        </compilerArgs>
                    </configuration>

regardless the IDE, we'll get tons of warnings and the compilation will fail. I see many of those warnings related to the new modules, but do we really need to address those now?

For instance:

class com.oracle.javafx.scenebuilder.kit.util.PaintConvertUtil in exported package com.oracle.javafx.scenebuilder.kit.util declares no explicit constructors, thereby exposing a default constructor to clients of module com.gluonhq.scenebuilder.kit

easily fixed with

    PaintConvertUtil() {
        // no-op
    }

Should we do that for all warnings (!!!) now?

[Oliver-Loeffler]

I was not worrying about all these warnings. It was indeed just those where you now already have added the transitive keyword, so that one can work using the kit module. However, the aether.api item is similar to the SkeletonConverter / SkeletonContext public API situation. There are many classes which expose parts of aether.api. Having them not exported (or declared transitive) may limit the usage if kit.

We export many items of the kit maven package:

    ...
    exports com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven;
    exports com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.preset;
    exports com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.repository;
    exports com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.repository.dialog;
    exports com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.search;
    ...

And those items have public API refering to aether.api classes. Hence my proposal to declare `aether.api as transitive.
All the other warnings as you have mentioned, should be addressed in a separate PR.

Following kit classes expose API from aether.api:

• com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.search.LocalSearch - org.eclipse.aether.artifact.DefaultArtifact
• com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.search.MavenSearch - org.eclipse.aether.artifact.DefaultArtifact
• com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.search.Search - org.eclipse.aether.artifact.DefaultArtifact
• com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.search.SearchService - org.eclipse.aether.artifact.DefaultArtifact
• com.oracle.javafx.scenebuilder.kit.editor.panel.library.maven.MavenRepositorySystem - RemoteRepository, Version, etc.

To keep this accessible and working, despite leaking 3rd party aether.api, I would still keep aether.api as transitive module and would rework this later in a separate PR. By having this transitive in the first place we keep the kit module work as a whole.

There are many more warnings but will be subject to upcoming PRs.

[jperedadnr]

Right, I took the shortcut of exporting every possible package from Kit (most of them are used by App), and because I didn't want to impose any limitation on third party projects using Kit.

Based on the lint warnings, I've added the default missing constructors, and also the requires transitive for aether.api.

There are still a few warnings (58 for Kit, 7 for App, but few of them related to modules now, about using automatic-modules)
About lint warnings, I've filed #725 and for some more urgent fix: #726

[Oliver-Loeffler]

Looks good to me now. That was lots of work. Thanks @jperedadnr .

[Oliver-Loeffler]

There are still a few warnings (58 for Kit, 7 for App, but few of them related to modules now, about using automatic-modules) About lint warnings, I've filed #725 and for some more urgent fix: #726

I have also started a new issue #727 to address the problem with tests in Eclipse.