It is used in the following dialog:

I think, the reason is that we switched to the material font in #230 and decided to use only material icons there. So, either we adapt the interface to our material icons (see e.g., #1549 (comment)) or just drop the icon completely. As first step, I would rename icon to materialDesignIconCodePoint and drop icon completely.

You are correct. However, since it is not possible to edit the icon the removed methods are not needed.

I noted that I made a mistake though so this cannot be merged yet.

Once done it should be possible to split external into gui and logic in a way that makes sense.

Problem 1: the mapping between icon name and codepoint is in IconTheme in gui. As the codepoint is needed in ExternalFileType and ExternalFileTypes it would be nice to move the mapping outside of gui. However, it is not obvious to me if this can be obtained with the code looking as nicely as it is now.

I guess having a set of constant strings would be one option, so in IconTheme it looks like BLOG(IconMapping.BLOG) instead of BLOG("\uf46b") and in IconMapping (in logic) like public static final String BLOG = "\uf46b", but this isn't really nice. (Maybe one can use enum?)

Problem 2: The OpenOffice icon used was not from material design. Hence, there is currently no icon.

Problem 3: Migration is not really working yet. As the fifth argument earlier the icon name was saved (which wasn't really used anyway), now the code point is saved. An option may be to save a sixth argument saying if the fifth is icon name or code point. This will also solve problem 2.

The objective would be to move ExternalFileType and ExternalFileTypes to logic as a cleanup is dependent on them. The rest should move to gui (from a quick check).

Regarding the distinction of the icon names and code points: As far as I understand, code points always start with \, whereas I would expect icon names to never start with \. So a very simple detection would be: boolean isCodePoint = myIconOrCodePoint.startsWith("\\"); That could solve problem 2 and 3, right?

Regarding the refactoring of ExternalType / ExternalFileTypes to get them into logic: The problem is that the classes store an Icon. Wouldn't it be possible to replace this instance variable with the code point / icon name, which is just a String? Then, we would need a facility in gui that accepts an ExternalFileType and builds an icon from. I have no idea what the effort is for implementing this, though...

EDIT: Nevermind, just looking at the code I see that saving a String instead of an Icon is exactly what you have been doing. The problem is in IconTheme. I have a meeting now and will update my comment afterwards.

But changing the type from Icon to String, doesn't change the fact that the information is still GUI-related, does it? It's just isn't in-your-face anymore. If you really want to move some part of it to the logic package, then I see no other way then to split it as follows:

• ExternalFileType in logic without any icon information
• ExternalFileTypeWithIcon in gui, deriving from ExternalFileType and providing access to the icon
• Some conversion code which enriches a given ExternalFileType by the icon based on some hard-coded map

But this is probably not worth the effort. In fact, ExternalFileType seems to be used (almost?) exclusively by editors and file dialogs and thus could happily reside in gui, right? Do I overlook something?

@tobiasdiez : I did something similar with FileList half a year ago, splitting into a gui free version and an "extended" one, which was not an appreciated solution then. Yes, it is GUI related primarily, but there is logic code that uses it.

@lenhard : I did a move right now which is architecturally correct (except that I haven't bothered to remove Globals yet). Not nice, but the only way I see to enable moving parts to logic.

@tobiasdiez is right of course. If we remove the icon information from ExternalFileType completely, it could (and should) even go into model, since it is just a class that represents data.

The map to icons is currently stored in ExternalFileTypes, but it should be possible to extract that code to a new GUI class and, as far as I can see, this is the only GUI-specific code.

The ExternalFileTypes are mostly used in the gui, but there is some logic code (UpgradePdfPsToFileCleanup) and the pdfimport code that uses it. Also, we have ParsedFileField in model, which uses a fileType through which it indirectly links ExternalFileType via Strings. The problem seems to be somewhat subtle.

I think the cleanest solution would be to extract ExternalFileTyp into model and sort of duplicate it in gui to add the icon information to it. @JabRef/developers What do you think?

Btw.: We have the same problem with special fields, which are in a sense model, but store an Icon.

Edit: @oscargus My comment came to late and I did not see yours before, sorry! How about moving ExternalFileType and ExternalFileTypes into model?

The code point is actually not starting with \ as I understand it, it is just a way to enter unicode characters, so it should be stored as the character (still, the length of that is one character, not several). Haven't come that far though so I can confirm it.

@lenhard No worries. I did the latest edits without realizing that there were valuable input to the process... I guess ExternalFileType could reside in model for sure. ExternalFileTypes currently require localization. Somehow, extending ExternalFileType as you and @tobiasdiez suggest should also be OK (ExternalFileTypeWithIcon extends ExternalFileType with an additional Icon field), but it is not completely clear how to deal with ExternalFileTypes. Having two lists (ExternalFileTypes and ExternalFileTypesWithIcons) should be OK, the question is more how to generate them... Not really clear how to deal with it as the logic one is easily created from the gui one, but a call to getInstance() in the logic one, cannot get information from the gui one... This can of course be solved by a call to getInstance() early in the code, although that is not really the preferred approach... The risk of duplicating to much is that we will need to edit it in two places...

SpecialFields is indeed a similar issue that needs to be resolved.

An alternative approach is maybe to hard code the file types for ps and pdf (PS and PDF), which is really what ExternalFileTypes is used for in UpgradePdfPsToFileCleanup. One might think based on the class name that it is enough to cover ps and pdf files... (It is also used for evostar_pdf in the migration but it may be OK to drop that now I think, no idea what it is and no default support for it apart from this.)

@oscargus: Hard coding ps and pdf in the upgrade functionality and dropping evostar_pdf seems reasonable to me! After all, the fields are already "hard-coded" in the methods name.

Regarding the duplication, I fail to come up with a satisfying solution at the moment...

So this PR is superfluously after #1934?