atarw / material-ui-swing

A modern, Material Design UI for Java Swing
MIT License
653 stars 86 forks source link

Rearrange classes & packages to allow usage from modular apps (JPMS) #107

Closed metteo closed 4 years ago

metteo commented 4 years ago

Hi

I tried using your awesome library in my modular project (Java 11) but it failed because of few issues:

  1. MaterialUISwingDemo.java is in a default package. This is unacceptable in modular / published libraries (major)
  2. The JAR published to maven central is actually a full assembly with all dependencies (major)
  3. There is no module-info.java or at least "Automatic-Module-Name" in MANIFEST.MF (minor)

I could create a PR with the changes but wanted to consult first.

IMHO the Demo class should be a separate git repo / maven module depending on your preference. Such module could output a JAR with dependencies to easily present the features, however the main library artifact should only point to dependencies using maven.

When it comes to module name it's a good practice to reserve the module name using manifest entry and later when switching to full JPMS, include module-info.java file.

Based on this great article: Module naming by @jodastephen I would suggest: mdlaf (which is not perfect as a package/module name but will have to do)

What do you think?

vincenzopalazzo commented 4 years ago

Hi @metteo,

There are a lot of good points in this issue, but before I want toadd a link to the recently material-ui-swing repository, material-ui-swing.

material-ui-swing last year started using Gradle and left maven.

I will return to your good point.

I could create a PR with the changes but wanted to consult first.

This library is life yet because the community work together, all request/change/talk are appreciated.

What do you think?

I know about this "structural error", unfortunately. I'm working on this library for about 4 years in the free time and I'm working and study how optimizing the behaviors of the components. How you can note this library doesn't be a "static" library but the final developer can decide the style with this library, as a personal theme, or the style of the component. For this reason, I didn't have time to make a modular project and I'm happy to receive help.

MaterialUISwingDemo.java is in a default package. This is unacceptable in modular / published libraries (major)

I can move today this class inside the package test. This is a good point, at this moment MaterialUISwingDemo.java helps me only to make a rapid test when I make the change.

The JAR published to maven central is actually a full assembly with all dependencies (major)

Right. The jar contains swinx1.6 how dependence because the library contains one component of the swingx library. It is good to have the material-ui-swing-swingx submodule.

There is no module-info.java or at least "Automatic-Module-Name" in MANIFEST.MF (minor)

The library doesn't is designed under modules structure, unfortunately.

IMHO the Demo class should be a separate git repo / maven module depending on your preference.

We have the demo with a separate git repository, here, the demo inside the library is an important demo if you want developer the component, the swing lifecycle is complex and not exist documentation about it. The only method to try to implement a UI Component is made a test until the component developing.

Based on this great article: Module naming by @jodastephen I would suggest: mdlaf (which is not perfect as a package/module name but will have to do)

Good point and I'm happy to improve the package name but this is a hard fork and I don't want to break the 200 client projects at the moment. I think this is a change to do in a major version like 2.0.

Conclusion

I'm happy to create the modular version of the library with you is this period, but all this change has a separate milestone:

What do you think about this point? if you want start to introduce the modular stucture, I can create a new branch called v1.2 and you can start to work inside it.

metteo commented 4 years ago

Thanks for the response.

I agree that the changes proposed should end up in a new minor release (1.2):

  1. MaterialUISwingDemo.java to be moved to src/test/java
  2. Use gradle publishing of maven artifacts to deploy the library to Maven Central (with properly generated pom.xml). The library would not be an assembly with runtime dependencies but instead include them in the pom.
  3. Include Automatic-Module-Name: mdlaf in MANIFEST.MF

These changes would be sufficient to make a valid artifact (for both Java 8 class path and Java 9+ module path use cases).

When it comes to modularization of themes, I think that we should hold until 2.0: removing classes from the library is a breaking change after all, even though they end up in some other module.

P. S. Sorry for writing in the wrong fork. If yours (vincenzopalazzo/material-ui-swing) is the main one then consider detaching it from the parent fork (GitHub support needs to be contacted in such case)

vincenzopalazzo commented 4 years ago

Hi @metteo,

MaterialUISwingDemo.java to be moved to src/test/java

This change that I will today in the release rc2 will be released in this week.

P. S. Sorry for writing in the wrong fork. If yours (vincenzopalazzo/material-ui-swing) is the main one then consider detaching it from the parent fork (GitHub support needs to be contacted in such case)

Don't worry, I managed this repository too, I will allineate the change for arch major release.

This is the description inside the readme.md of this repository

This is the official repository of material-ui-swing but the developing of a new feature we do in this in this repository vincenzopalazzo/material-ui-swing and after testing the official release will release in this branch.

I hope to have time the change that you suggest

metteo commented 4 years ago

@vincenzopalazzo I tried rc3 and the changes with the demo class and manifest entry are visible.

Unfortunately the fact that it's an assembly with all the dependencies inside, is still a blocker. I guess you could upload 2 artifacts for 1.1.1 release:

Clients who don't use gradle / maven would have to start using the second one for their projects.

In the future when you split the themes into separate artifacts you could introduce another one:

vincenzopalazzo commented 4 years ago

Hi @metteo I pushed now the rc4 this version contains your changes pushed with this PR

Release description here

Thank you.

metteo commented 4 years ago

I can confirm this issue is fixed. Screenshot from chip8-swing below:

screenshot

Thanks for help!

vincenzopalazzo commented 4 years ago

Hi @metteo, Your screenshot looks very good, I hope to play to snake game with your emulator :)