Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

UserLibrary: feature for adding folders to library #228

Merged
merged 7 commits into from
May 2, 2019
Merged

UserLibrary: feature for adding folders to library #228

merged 7 commits into from
May 2, 2019

Conversation

luca-domenichini
Copy link
Contributor

This PR adds a long waited (by me) feature, that enables the user to add a folder (in particular, root folders with classes) to the custom components library.

immagine

Users can add as many folders as they want.
After adding one folder to the library, that one is used as the root for loading classes in all its subfolders, and components that were able to be loaded are added to the "Custom" library panel, as if they loaded from a jar.

Bonus: the library label updates its text during scanning, so it is evident to users that a maybe long process is running..
immagine

The main advatange of this PR, is that a developer can add a "bin" folder from their running Eclipse/IDEA to the library manager and have the custom components loaded into SB without the need to create and publish a jar to the library directory everytime.

Feel free to ask for clarifications...

@luca-domenichini
Copy link
Contributor Author

Just a couple of implementation notes:

  1. class FolderExplorer was nearly cloned from JarExplorer, reusing JarReport and JarReportEntry: that was done in order to minizime the changes, but it would be better to create 2 base classes LibraryReport and LibraryReportEntry and have JarReport/Entry and FolderReport/Entry extends those bases classes

  2. Because of 1. i create LibraryUtil class, just to have some common methods inside it.. having the base classes as of 1., LibraryUtil is maybe useless.

Tell me if you want me to make some change. (anyway, I noticed that LibraryUtil is missing the license header...)

@jperedadnr
Copy link
Collaborator

jperedadnr commented Apr 7, 2019

This looks like a nice feature to be added to Scene Builder. Thanks for your PR!

I've tried it but I had some issues to make it work.

  • If you provide a folder that has a jar in it, it won't work, as it will explore only .class files, but won't explore the jar itself. Is this correct? Maybe this can be fixed.

  • If you provide a folder (bin, out, build...) that contains .class files, it doesn't work for me either. This was unexpected.

If you check this method: LibraryFolderWatcher::makeURLArrayFromPaths, the class path is configured only with jars:

result[i++] = new URL("jar", "", p.toUri().toURL() + "!/");

so when you add your folder (bin, out, build, ...) to the class path, it will include a jar that doesn't exist.
So I had to change it to make it work to:

try {
                URL url = p.toUri().toURL();
                if (url.toString().endsWith(".jar")) {
                    result[i++] = new URL("jar", "", url + "!/"); // <-- jar:file/path/to/jar!/
                } else {
                    result[i++] = url; // <-- file:/path/to/folder/
                }
            } ... 

That loads any jar and also any folder with the usual file protocol.

The same applies to ImportWindowController::makeURLArrayFromFiles. Otherwise, the default Import Dialog won't find the control, and will show "Import Jar (no UI components)" caption.

Next issue: You need to provide the exact folder that has the classes based on their package name.

For instance, for a control under /path/to/out/production/com/mycompany/control.class

  • this doesn't work: /path/to/out/
  • but this works: /path/to/out/production.

While this is okay, somehow it should be advised. Or ideally, some kind of recursion algorithm could be used.

Another possible issue: what happens with dependencies? When we import a control from a jar, all traverse dependencies are resolved, but how does it apply in this case?

@luca-domenichini
Copy link
Contributor Author

luca-domenichini commented Apr 8, 2019

Hello Josè, thanks for the reply.

Just a quick reply (I'm in a hurry within a meeting..):

Exploring jars inside the specified folder wasn't supposed to work: just .classes. Probably you are right it could be interesting, but I would add this functionality with another PR; My aim is just to add capability to work with the binaries compiled by IDEs.

Let me check in more detail the dependencies issue. I will reply ASAP.

And yes, you have to specify the root folder of the classes you want to add. Do you think it could be worth trying to make class name with parents directories too?

bye

@luca-domenichini
Copy link
Contributor Author

luca-domenichini commented Apr 8, 2019

Well, I've tested current dependency discovery feature for directory-based library and jar-based, and have found that it almost works as expected. I have a test case that fails to load a custom component (using SB 8.5.0 both gluon-official and my branch), but I'll post an issue for that; this affects both jar-based library components both directory-based, and is quite simple to reproduce: just add some code in custom component that uses an external library that is not available to SB (e.g. org.springframework.context) and you will get something like this:
Exception for: MyButton.class java.io.IOException: java.lang.NoClassDefFoundError: org/springframework/context/ApplicationContext at com.oracle.javafx.scenebuilder.kit.library.util.FolderExplorer.instantiateWithFXMLLoader(FolderExplorer.java:116) ...

Can you elaborate better, please?

In the meanwhile, i'm going to push the fixes you suggested (and thanks for that).

@luca-domenichini
Copy link
Contributor Author

luca-domenichini commented Apr 8, 2019

Do you think this should be better?
immagine

@luca-domenichini
Copy link
Contributor Author

@eduarddrenth maybe you could be interested in this?

@eduarddrenth
Copy link

Once I start developing FX applications again, this sounds like a feature I would use yes. Only thing is I do mostly EE/web based these days.

@FilloFolla
Copy link

Great @mimmoz81. It's a good and useful work!

@luca-domenichini
Copy link
Contributor Author

@jperedadnr any news?
do you want me to do something else about this?

@jperedadnr
Copy link
Collaborator

jperedadnr commented Apr 22, 2019

Maybe we should try to shorten the text, something like:

Add root folder with *.class files

And maybe you can add a tooltip for a more verbose explanation?

ADD tooltip for folder hyperlink
@luca-domenichini
Copy link
Contributor Author

Here you are
immagine

@jperedadnr
Copy link
Collaborator

Thanks, before we can merge this PR, would you mind signing this CLA?

@luca-domenichini
Copy link
Contributor Author

Signed

@jperedadnr jperedadnr merged commit fd47d22 into gluonhq:master May 2, 2019
@jperedadnr
Copy link
Collaborator

@mimmoz81 Hey, as we have already merged this PR, I've started with a new one, that will provide logging to report the result of importing custom controls from jar or folders: #234

I've started also some refactoring, so feel free to review and test.

@luca-domenichini
Copy link
Contributor Author

Hello, thanks for merging this!

I'm preparing other useful stuffs about custom controls: I'll do another PR ASAP; if you are planning a release soon, please tell me when so that this new PR will get merged too.

In the meanwhile, I'll check your modifications.

@UltraBurstXD
Copy link

Why is this PR not available for Linux Scene Builder too?

@luca-domenichini
Copy link
Contributor Author

Do you mean it does not work on Linux machine or it is simply missing from binary release?

@UltraBurstXD
Copy link

UltraBurstXD commented Dec 15, 2020

It's missing...? Don't know for sure:
Screenshot from 2020-12-15 16-40-25

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants