Extend architecture tests to javafx and javafx.collections

[lenhard]

Extends the architecture tests to also cover javafx and javafx.collections, as discussed in #2617.

It seems that it is high time for this, since javafx classes are already being misused, as this PR uncovers. Thus, this currently fails the build. The misuses are:

• org.jabref.logic.exporter.GroupSerializer/GroupSerializerTest/GroupsParserTest uses javafx.scene.paint.Color
• org.jabref.logic.journals.Abbreviation uses javafx.beans.property.SimpleStringProperty
• test/org.jabref.logic.l10n.LocalizationParser uses FXMLLoader/PlatformImpl

Although we might think about making exceptions for the beans and tests, I don't see why the Color class should be directly accessed in the gui.

• Change in CHANGELOG.md described
• Tests created for changes
• Screenshots added (for bigger UI changes)
• Manually tested changed features in running JabRef
• Check documentation status (Issue created for outdated help page at help.jabref.org?)
• If you changed the localization: Did you run gradle localizationUpdate?

[tobiasdiez]

Thanks for having a look at it. The code looks good.
Concerning your points:

• Color: was for me a model class (similar to awt.geom) and we don't have to work with the color string. The class has no methods to draw etc but is just a wrapper around three integer. Yes, of course, color is inherently connected to GUI concepts. Anyway, if you really dislike it then it should be possible to store the color string in the model class and convert it to Color in the GUI. But the same GUI-related information will still be present in the model...it is just stored as string. I personally find this worse.

• beans: I think an exception would be fine since it is simlar to collections. But this is more a fundamental question about bean properties vs old-style getters and setters, with eventbus in the model/logic. I like the beans more than the eventbus, but this is personal preference.

• tests: the localization tests need to parse the FXML files to extract the used resource keys; there is no way around it.

[lenhard]

Thanks for your reply:

• Looking at the Color class, it is just a representation of the RGB code, so not too problematic. I am still not 100% sure. Assuming we would switch to a new GUI technology, would we be using javafx.Color objects, or would we have to refactor the model classes to only use the RGB code string? I would really like to have a third opinion from someone from @JabRef/developers on this, so that we can sort of say that we have a majority consensus.
• I don't think very highly of our custom event bus (and have always said so), so bean properties would be acceptable to me, especially considering the fact that we allow observable collections, which are essentially just "bean data structures"
• It might make sense to exclude the whole test sources from the architecture tests. Imho, they are not really relevant for static architecture checks. I will have a look at that.

[lenhard]

I have been thinking a little more about Color. Isn't this the very same thing as we have in https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/IconTheme.java ?

There, we also have all the color codes, but in the gui package and not in model. So what is our reasoning to allow the javafx version of Color in model, but not the awt version? Also, had we used the awt version in model, wouldn't that have meant that we would have had to rewrite all such usages for the javafx migration? If yes, isn't that a good argument for not allowing the javafx version of Color in model?

And interestingly, I found this: https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/logic/groups/DefaultGroupsFactory.java#L13 This is a plain violation of our architecture rules. @tobiasdiez Did you do the import like that on purpose, so that the tests do not find it, or was that an accident?

[tobiasdiez]

I think the issue with the Color class is that previously there was no need to have anything color-related in the model as no class in model relied upon it - thus there was never situation where the usage of awt.color in model was on the table. From an architectural standpoint, the cleanest solution is probably to implement our own version of Color in model and use this. The gui contains then a converter from this Color to whatever color is used in the corresponding UI framework. However, this is additional work and apart from making a migration to a new GUI easier I don't see any advantages.
@simonharrer as I have our discussion about various architectural points in good memory, I would highly value your opinion on this matter (if you find the time to look into it).

I can't remember that I was creative enough to use the full path to hide the dependency from our architecture tests. But good to know that this works, I will now definitely (ab)use it and make my life way easier 😄 . Of course I will fix it (tomorrow).

[koppor]

@tobiasdiez The fix will go into this PR?

[koppor]

• Color: +1 to add it as exception, because it is "just" a data model. I would not implement a separate color class, even though this would be the cleanest solution. (Effort saving)
• Beans: +1 to allow them
• Tests: Yes, please exclude them.

[koppor]

Side comment: Macker seems to be a tool, which also implements architecture checks. Example: https://innig.net/macker/example/layering/src/macker.xml. OK, it is outdated, but I thought, it is nice to know. 😇

[lenhard]

I still think that making an exception for Color is a bad decision that will come back to bite us. In the course of moving to JavaFX, we are currently losing a lot of independence in our model packages - something that we invested a lot of effort when trying to get rid of the swing dependencies.

But if this is the consensus in the team, I'll bow to that. I created exceptions for the remaining FX classes and this PR should be ready to go when the build succeeds on travis.