Fix custom exporter not displayed in file chooser #4015

Siedlerchr · 2018-05-06T10:23:51Z

Fixed several issues with the Custom Exporter.

Filename check for layout files was wrong
Commandline name for custom exports was the filename and not the name, opposed to what our help says
Custom exports could not be added due to our hardcoded enum
Expanded the enum with Mixins

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?)

Rename FileType -> BasicFileType Fix error with layout file in custom export Fix problem with custom export and file types Remove old ExportFileFilter Importer still uses BasicFileType

tobiasdiez

I just had a quick look but I'm a bit confused. The problem that custom export formats are not displayed correctly sounds like a gui problem to me. However, the solution involves redefining / changing a lot of logic code. Thus I think there should be an easier solution.

The exporter are displayed using the FileChooser.ExtensionFilter. Thus I suspect that one only needs to change the way how an exporter is converted to an ExtensionFilter. For the "All import formats" option, this is done in

jabref/src/main/java/org/jabref/gui/util/FileFilterConverter.java

Lines 47 to 49 in e32bdc8

    
           public static FileChooser.ExtensionFilter forAllImporters(SortedSet<Importer> importers) { 
        
               return convertImporter(Localization.lang("Available import formats"), importers); 
        
           }

and I don't see why such an approach shouldn't work also for custom exporters.

But as I said, I only had a quick look and so probably missed something obvious.

tobiasdiez · 2018-05-06T20:30:31Z

src/main/java/org/jabref/logic/util/BasicFileType.java

+        allfileTypes.add(fileType);
+    }
+
+    public static FileType parse(String description) {


It appears to me that the parse method is no longer needed and I suspect that hence the allFileTypes list can be removed. Right?

Yes missed that.

Siedlerchr · 2018-05-06T21:23:41Z

The core problem is not the gui but that each exporter has a file type and this file type was also added for the custom exporter. These file type used the parse method to find a file type based on the extension. The file type class is used for the file filter chooser. And an exporter is matched on the file type description.
But as this is an enum, it did not work for custom export as always one of the enum values was used, e.g the default HTML Export.

The core changes are in the enum which now implemented an interface.
In fact it's just a rename of the old enum and a simple interface which has the name of the old one

To avoid code duplicates in the anonymous class and the enum I moved them to private static methods.

tobiasdiez · 2018-05-11T22:10:13Z

ok, thanks for the explanation. I thought a bit more about this problem and came to the conclusion that the cleanest solution is probably to split the description from the "file type". If the later is only a list of extensions (xml, bib, txt, ...) and the importer/exporter itself contains the description, then you can easily create a ExtensionFilter for an importer/exporter (and don't need to pay special attention to custom exporters). What do you think?

Siedlerchr · 2018-05-12T07:15:51Z

This would only work if the custom export follows the standard file types. If I want my exporter to have filetype xyz then this would not be possible.
What I did is to provide a set of standard file types and their descriptions which can be extended to support custom one. The standard file types are also used for the importer.

Another thing which just came to my mind was that we with this approach could also resuse the functionality for the ExternalFileType stuff. Than we could get rid of that completely.

tobiasdiez · 2018-05-12T22:08:56Z

ok, good point. Do you want to improve the code around ExternalFileType as part of this PR or is this a follow-up?

I would still separate the file extension from the description text.

Siedlerchr · 2018-05-12T22:19:16Z

I would rather do this in a follow up on the maintable directly

tobiasdiez · 2018-05-15T21:00:33Z

CHANGELOG.md

@@ -16,6 +16,10 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - We changed the implementation of the `[shorttitle]` key pattern. It now removes small words like `a`, `an`, `on`, `the` etc. Refer to the help page for a complete overview. [Feature request in the forum](http://discourse.jabref.org/t/jabref-differences-in-shorttitle-between-versions-3-8-1-and-4-not-discounting-the-a-an-of-in-titles/1147)

 ### Fixed
+We fixed an issue where the export to clipboard functionality could not be invoked [#3994](https://github.com/JabRef/jabref/issues/3994)
+We fixed an issue with the migration of invalid Look and Feels [#3995, comment](https://github.com/JabRef/jabref/issues/3995#issuecomment-385649448)


This looks like a duplication (probably due to a problem while merging).

tobiasdiez · 2018-05-15T21:02:25Z

src/main/java/org/jabref/gui/util/FileDialogConfiguration.java

@@ -101,7 +102,7 @@ public Builder withInitialDirectory(String directory) {
            return this;
        }

-        public Builder withDefaultExtension(FileType fileType) {
+        public Builder withDefaultExtension(BasicFileType fileType) {


I think this method should still accept FileType

tobiasdiez · 2018-05-15T21:03:05Z

src/main/java/org/jabref/gui/util/FileFilterConverter.java

        return toExtensionFilter(description, fileTypes);
    }

-    private static FileChooser.ExtensionFilter toExtensionFilter(String description, List<FileType> fileTypes) {
+    private static FileChooser.ExtensionFilter toExtensionFilter(String description, List<BasicFileType> fileTypes) {


Here also FileType

tobiasdiez · 2018-05-15T21:04:49Z

src/main/java/org/jabref/logic/exporter/TemplateExporter.java

@@ -117,7 +118,7 @@ public TemplateExporter(String consoleName, String lfFileName, String directory,
     * @param savePreferences Preferences for saving
     * @param deleteBlankLines If blank lines should be remove (default: false)
     */
-    public TemplateExporter(String consoleName, String lfFileName, String directory, FileType extension, LayoutFormatterPreferences layoutPreferences, SavePreferences savePreferences, boolean deleteBlankLines) {
+    public TemplateExporter(String consoleName, String lfFileName, String directory, BasicFileType extension, LayoutFormatterPreferences layoutPreferences, SavePreferences savePreferences, boolean deleteBlankLines) {


tobiasdiez · 2018-05-15T21:05:09Z

src/main/java/org/jabref/logic/importer/Importer.java

     */
-    public abstract FileType getFileType();
+    public abstract BasicFileType getFileType();


here as well...

tobiasdiez · 2018-05-15T21:06:48Z

src/main/java/org/jabref/logic/util/BasicFileType.java

+import org.jabref.logic.l10n.Localization;
+
+/**
+ * This enum contains a list of file types used in open and save dialogs.


We shouldn't reference dialogs here (as we are in the logic package).

tobiasdiez · 2018-05-15T21:13:09Z

src/main/java/org/jabref/logic/util/BasicFileType.java

+    DEFAULT(Localization.lang("%0 file", "DEFAULT"), "default");
+
+    private static Set<FileType> allfileTypes = new HashSet<>(EnumSet.allOf(BasicFileType.class));
+    private final String[] extensions;


It probably makes the code below a bit easier to read if you simply store the extensions as a List. Moreover, the comment below can probably be removed.

tobiasdiez · 2018-05-15T21:14:39Z

src/main/java/org/jabref/logic/util/FileType.java

-    public String getFirstExtensionWithDot() {
-        return "." + extensions[0].trim();
-    }
+    String getFirstExtensionWithDot();


For these two method you can provide a default implementation.

tobiasdiez · 2018-05-15T21:16:59Z

src/main/java/org/jabref/logic/util/BasicFileType.java

+    }
+
+    public static FileType addnewFileType(String description, String... extensions) {
+        FileType fileType = new FileType() {


It is probably better to extract this anonymous class to a separate class since it looks to be the standard implementation that can be reused in custom importers

I have created a factory method for this. So the caller does not need to create it's own file type objects
public static FileType addnewFileType(String description, String... extensions) {

tobiasdiez · 2018-05-15T21:20:21Z

src/main/java/org/jabref/logic/util/FileType.java

-        }
-        return sj.toString();
-    }
+    String getDescription();


Also on a second look, I'm strongly in favor of removing the description from the file type. It is actually the description of the exporter/importer that we should show in the dialog...if you want to reuse the strings you can extract the descriptions in BasicFileType to a new class, but its also ok if you just repeat the description in the importer and exporter.

This would not solve the underlying problem and in fact would result in the same problem I intend to fix here.
As we have multiple file types with the same extension e.g. html. We already have Html Table, Html LIst etc. Only with the added description, the file type is unique

I think your example is a good illustration why the current mix of file extensions and description is misleading. The file type is html in both examples, but the exporter used is different (tables vs list). You don't get these kind of problems if the FileType is only an abstraction of the extension and the description comes from the exporter...

At least we need the description in the extension filter. Otherwise we can't map it. I will think about this. I will look and see how this would affect the importer

…leType * upstream/master: (33 commits) Import inspection uses now same font size setttings as maintable (#4062) Add date checker (#4007) Enable tests macOs push to application fix Fix #4041 to make Push to Application work again on OSX. (#4057) Add NormalizeEnDashesFormatter (#4045) Package private for tests Shutdown duplicate code Move migrations to PreferencesMigrations Structure startup Extract migrations A first solution for the cli problem #4038 (#4047) New translations JabRef_en.properties (French) (#4052) Update CHANGELOG.md Fix checkstyle Make test more informative in the failing case New translations JabRef_en.properties (French) (#4051) New translations JabRef_en.properties (Vietnamese) (#4050) Make Formatter an abstract class (and remove AbstractFormatter) New Crowdin translations (#4048) ... # Conflicts: # CHANGELOG.md

remove list of custom file types as it's not needed use list instead of varargs params

…abref into fixCustomExportFileType * 'fixCustomExportFileType' of https://github.com/JabRef/jabref: # Conflicts: # CHANGELOG.md

…leType * upstream/master: New Crowdin translations (#4065) New translations Menu_en.properties (Vietnamese) (#4063)

…leType * upstream/master: Update guava and checkstyle (#4066)

Siedlerchr · 2018-05-31T14:29:15Z

@tobiasdiez I now removed the exporter stuff from the FileTypes. So currently it's only used for the importers. The key problem with the importers is, that we have an "All available import formats" plus all the extensions. In this case I see no way to filter the importer without having a description in the extension format.

tobiasdiez · 2018-05-31T14:49:00Z

A shot in the blue: Instead of

jabref/src/main/java/org/jabref/gui/util/FileFilterConverter.java

Lines 26 to 29 in e32bdc8

    
           private static FileChooser.ExtensionFilter convertImporter(String description, Collection<Importer> formats) { 
        
               List<FileType> fileTypes = formats.stream().map(Importer::getFileType).collect(Collectors.toList()); 
        
               return toExtensionFilter(description, fileTypes); 
        
           }

try

 private static FileChooser.ExtensionFilter convertImporter(Importer importer) { 
     return toExtensionFilter(importer.getDisplayName(), Collections.singletonList(importer.getFileType)); 
}

and at the same time change

jabref/src/main/java/org/jabref/gui/util/FileFilterConverter.java

Lines 39 to 41 in e32bdc8

    
           public static Optional<Importer> getImporter(FileChooser.ExtensionFilter extensionFilter, Collection<Importer> importers) { 
        
               return importers.stream().filter(importer -> importer.getFileType().getDescription().equals(extensionFilter.getDescription())).findFirst(); 
        
           }

to

 public static Optional<Importer> getImporter(FileChooser.ExtensionFilter extensionFilter, Collection<Importer> importers) { 
     return importers.stream().filter(importer -> importer.getDisplayName().equals(extensionFilter.getDescription())).findFirst(); 
}

If getImporter returns an empty optional, you know that there is no specific importer, i.e. the user selected "any file" or "any import format".

only keep those with multiple extensions TODO: convert each importer

group extensions

Siedlerchr · 2018-05-31T18:52:45Z

@tobiasdiez I noticed that it was already possible. I just needed to create the Map with Description/Name and FileType like I did for export. It is now analogous to export. I removed the not needed FileTypes.
However, there are still some import formats left which have more than one extension. However, I think it's not worth changing them, too. Would make things a lot more complicated.
We already have a good base for future expansions and for using this in combination with ExternalFileTypes.

tobiasdiez

Very good! There are still some unimplemented comments from previous reviews (the only big one being that the description part should now be removed from the filetype interface).

tobiasdiez · 2018-05-31T20:28:41Z

src/main/java/org/jabref/gui/exporter/ExportAction.java

@@ -152,7 +155,7 @@ public void update() {
    }

    private static FileDialogConfiguration createExportFileChooser(ExporterFactory exportFactory, String currentDir) {
-        List<FileType> fileTypes = exportFactory.getExporters().stream().map(Exporter::getFileType).collect(Collectors.toList());
+        Map<String, FileType> fileTypes = exportFactory.getExporters().stream().collect(Collectors.toMap(Exporter::getDisplayName, Exporter::getFileType));


The logic to convert an exporter (and importer below) to a FileExtensionFilter should reside in FileFilterConverter.

No, in this case it would not have any additional benefit. It would still return a map. The map is internally handled in the file dialog configuration and there handled.

…leType * upstream/master: Show development information Release v4.3 Remove invalid test and fix another CPU leak #3943 (#4058) # Conflicts: # CHANGELOG.md

koppor · 2018-06-03T11:52:57Z

If I understand correctly, this should go into master before merging #3621.

Siedlerchr · 2018-06-03T14:32:23Z

Yes definitely.

tobiasdiez · 2018-06-03T18:30:46Z

I've refactored a bit more (it was easier to hijack this PR than describe the refactorings in detail -sorry). If you are happy with these changes, feel free to merge; otherwise just revert my commits...

Siedlerchr · 2018-06-03T18:41:01Z

Looks good so far. If someone can merge this with co authoring ( i got this wrong last time)

Siedlerchr added 2 commits May 6, 2018 12:21

Fix custom exporter not displayed in file chooser

6844879

Fixes #4013

add changelog

3d4ebb6

Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 6, 2018

Siedlerchr added 2 commits May 6, 2018 21:50

Add Interface to extend FileTypes

03f544d

Rename FileType -> BasicFileType Fix error with layout file in custom export Fix problem with custom export and file types Remove old ExportFileFilter Importer still uses BasicFileType

fix checkstyle

1a0cde3

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 6, 2018

tobiasdiez requested changes May 6, 2018

View reviewed changes

Merge branch 'master' into fixCustomExportFileType

f66d27c

Merge branch 'master' into fixCustomExportFileType

289c439

tobiasdiez requested changes May 15, 2018

View reviewed changes

Siedlerchr mentioned this pull request May 17, 2018

Custom exports not working #4036

Closed

Siedlerchr added 5 commits May 27, 2018 17:22

replace BasicFileType with FileType

b275ad4

remove list of custom file types as it's not needed use list instead of varargs params

Merge branch 'fixCustomExportFileType' of https://github.com/JabRef/j…

e8adeda

…abref into fixCustomExportFileType * 'fixCustomExportFileType' of https://github.com/JabRef/jabref: # Conflicts: # CHANGELOG.md

Merge remote-tracking branch 'upstream/master' into fixCustomExportFi…

d348ac0

…leType * upstream/master: New Crowdin translations (#4065) New translations Menu_en.properties (Vietnamese) (#4063)

first part of extracting the file types for exporter from the enum

68ed0f8

Siedlerchr mentioned this pull request May 29, 2018

CPU leak #4058

Merged

stefan-kolb added this to the v4.3 milestone May 30, 2018

Siedlerchr added 3 commits May 31, 2018 13:04

Merge remote-tracking branch 'upstream/master' into fixCustomExportFi…

9c7b132

…leType * upstream/master: Update guava and checkstyle (#4066)

remove export file types from enum

26804cb

use display name instead of extension to map

983d158

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 31, 2018

Siedlerchr added 3 commits May 31, 2018 20:18

remove unused importers and exporters

834ba84

only keep those with multiple extensions TODO: convert each importer

extract import file dialog configuration analog to export

8df361a

merge pdf file types

cfb2613

group extensions

tobiasdiez requested changes May 31, 2018

View reviewed changes

Siedlerchr added 5 commits June 1, 2018 18:52

Merge remote-tracking branch 'upstream/master' into fixCustomExportFi…

4c07cd0

…leType * upstream/master: Show development information Release v4.3 Remove invalid test and fix another CPU leak #3943 (#4058) # Conflicts: # CHANGELOG.md

remove description from file type and interface

bd353ab

checkstyle

b8a33ad

checkstyle

bd37390

fix test

0bee0d4

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 3, 2018

koppor mentioned this pull request Jun 3, 2018

[WIP] Main table JavaFX migration #3621

Merged

35 tasks

tobiasdiez added 2 commits June 3, 2018 20:03

Move extension filter creation to FileFilterConverter

2ad3d3a

Code cleanup and rename BasicFileType to StandardFileType

8b14add

koppor merged commit 2f5f2e5 into master Jun 3, 2018

koppor deleted the fixCustomExportFileType branch June 3, 2018 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix custom exporter not displayed in file chooser #4015

Fix custom exporter not displayed in file chooser #4015

Siedlerchr commented May 6, 2018 •

edited

Loading

tobiasdiez left a comment

tobiasdiez May 6, 2018

Siedlerchr May 6, 2018

Siedlerchr commented May 6, 2018

tobiasdiez commented May 11, 2018

Siedlerchr commented May 12, 2018 •

edited

Loading

tobiasdiez commented May 12, 2018

Siedlerchr commented May 12, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

tobiasdiez May 15, 2018

Siedlerchr May 27, 2018

tobiasdiez May 15, 2018

Siedlerchr May 27, 2018

tobiasdiez May 27, 2018

Siedlerchr May 27, 2018

Siedlerchr commented May 31, 2018

tobiasdiez commented May 31, 2018

Siedlerchr commented May 31, 2018

tobiasdiez left a comment

tobiasdiez May 31, 2018

Siedlerchr Jun 1, 2018

koppor commented Jun 3, 2018

Siedlerchr commented Jun 3, 2018

tobiasdiez commented Jun 3, 2018

Siedlerchr commented Jun 3, 2018

	public static FileChooser.ExtensionFilter forAllImporters(SortedSet<Importer> importers) {
	return convertImporter(Localization.lang("Available import formats"), importers);
	}

Fix custom exporter not displayed in file chooser #4015

Fix custom exporter not displayed in file chooser #4015

Conversation

Siedlerchr commented May 6, 2018 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented May 6, 2018

tobiasdiez commented May 11, 2018

Siedlerchr commented May 12, 2018 • edited Loading

tobiasdiez commented May 12, 2018

Siedlerchr commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented May 31, 2018

tobiasdiez commented May 31, 2018

Siedlerchr commented May 31, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Jun 3, 2018

Siedlerchr commented Jun 3, 2018

tobiasdiez commented Jun 3, 2018

Siedlerchr commented Jun 3, 2018

Siedlerchr commented May 6, 2018 •

edited

Loading

Siedlerchr commented May 12, 2018 •

edited

Loading