Avoid erroneous MaterialDesignIconFactory

[r0light]

Fix for #5245

Refactoring of the code for managing icons. Now all usages of de.jensd.fx.glyphs or de.jensd.fx.glyphs.materialdesignicons are encapsulated in the package gui.icon.
In there, the usage of de.jensd.fx.glyphs.materialdesignicons.utils.MaterialDesignIconFactory is avoided, because of the error described in the issue.

---

• Change in CHANGELOG.md described
• Tests created for changes
• Manually tested changed features in running JabRef
• Screenshots added in PR description (for bigger UI changes)
• Ensured that the git commit message is a good one
• Check documentation status (Issue created for outdated help page at help.jabref.org?)

[tobiasdiez]

Thanks for your PR. That looks already pretty good.

Two suggestions for improvement:

• I would prefer if no classes use the MaterialDesignIcon class directly. For example, instead of MaterialDesignIcon.BRIEFCASE_CHECK one should use IconTheme.AUTO_LINKED_FILE. This makes it possible to globally change the icon of a feature without having to worry about where it is used.
• This requires also changes to JabRefMaterialDesignIconFactory (and maybe other classes) as instead of GlyphIcons one should now pass a JabRefIcon to the method (and one can use JabRefIcon.getGraphicNode to get the icon graphic).
• There should be an architecture test ensuring this dependency requirement of the form

    @ArchTest
    public static void doNotUseGlyphsDirectly(JavaClasses classes) {
        noClasses().that().resideOutsideOfPackage("org.jabref.gui.icon")
                   .should().accessClassesThat().resideInAPackage("de.jensd.fx.glyphs..")
                   .check(classes);
    }

(not tested and to be added here https://github.com/JabRef/jabref/blob/master/src/test/java/org/jabref/architecture/MainArchitectureTestsWithArchUnit.java)

Sorry, I could have thought about this in more detail in the morning and hope you don't have to make changes twice now.

[r0light]

Ok, thank you for your suggestions. I reworked it now and tried to improve it further by keeping all things related to MaterialDesignIcons inside gui.icon. And outside of this package only JabRefIcons are used. That also made the class JabRefMaterialDesignIconFactory obsolete.

It is working now as expected, but there is one thing which should be improved:
In the class org.jabref.logic.groups.DefaultGroupsFactory there was an import of de.jensd.fx.glyphs.materialdesignicons.MaterialDesignIcon which passed the architecture tests. When i refactored it, i exchanged it with an import of org.jabref.gui.icon.IconTheme which caused the architecture test to fail. Now I'm not sure how exactly the functionality in DefaultGroupsFactory is needed and don't know how to properly do it. To let the test pass, i used a String constant now...

I also added the architecture test you suggested and hope that it is correct, as i haven't done anything with ArchUnit yet :)

[Siedlerchr]

Looks good. But that workaround is no longer necessary?

[r0light]

Looks good. But that workaround is no longer necessary?

It's not a workaround anymore, but more a refactoring which also fixes the issue.
I would also say, that it is ready to merge, there is just the thing i described:

In the class org.jabref.logic.groups.DefaultGroupsFactory there was an import of de.jensd.fx.glyphs.materialdesignicons.MaterialDesignIcon which passed the architecture tests. When i refactored it, i exchanged it with an import of org.jabref.gui.icon.IconTheme which caused the architecture test to fail. Now I'm not sure how exactly the functionality in DefaultGroupsFactory is needed and don't know how to properly do it. To let the test pass, i used a String constant now...

[Siedlerchr]

I am going to merge this now. Looks good to me. I think the issue with the GroupFactory is when you store and read groups with an icon?