Fix #1771 Show all supported import types as default file filter #1774
Conversation
stefan-kolb
added
the
status: ready-for-review
stefan-kolb
force-pushed
the
import-dialogs
branch
from
37deaaa
to
0f09aba
Compare
|
@Siedlerchr This is one for you to review 😄 |
stefan-kolb
force-pushed
the
import-dialogs
branch
from
0f09aba
to
06d0753
Compare
| Localization.lang("Import"), JOptionPane.ERROR_MESSAGE); | ||
| return; | ||
| SortedSet<ImportFormat> importers = Globals.IMPORT_FORMAT_READER.getImportFormats(); | ||
| List<FileExtensions> extensions = importers.stream().map(p -> p.getExtensions()).collect(Collectors.toList()); |
There was a problem hiding this comment.
I would rather use the ImportFileFilter and there add a new constructor overload which accepts a List of Import Formats, e.g. just moving the Stream stuff in the ImportfileFilter, because it internally uses already the
Would make it here much cleaner. And in the dlg class you just call SetFileFilter(importFF)...
Maybe in future we can recycle that for the Export formats, too...
There was a problem hiding this comment.
Hm, I don't really get the idea with this text. Adds way more complexity in my opinion. I need a new Import format then and matching FileExtensions etc pp?!
There was a problem hiding this comment.
Well, the ImportFileFilter is an custom already exiting FileFilter, which maps the extension to the import format.
It internally calls the accept method of the method of an ExternalFileFilter. See #1775 which you already merged in...
If you stay with your solution, then you could remove it completely.
There was a problem hiding this comment.
I know that but I need 2 new Objects for getting that to work, a FileExtension enum with all import endings and a new Import Format class for all Import Files?! This is a little bit complex to implement as the number of import formats can change. or am I on the wrong track here?
There was a problem hiding this comment.
Not sure if I get the point of your discussion, but in general I support the idea that FileDialog should support a way to set the extensions and file filter based on import formats (because this should happen quite often).
What about adding a static FileDialog createDialogForImport(frame, workingDir, Collection<ImportFormat>) which contains most of the initialization code here.
There was a problem hiding this comment.
@stefan-kolb You are thinking a bit too complex ;)
We already have those two thing in the ImportFormat class.
The importFormat has a method getExtensions wich returns the asscoatiacted enum value from the FileExtensions class.
Currently in the importFileFilter we have support for exaclty one importFormat.
There was a problem hiding this comment.
A ok, so youre thinking about public ImportFileFilter(String name, List<ImportFormat> formats). I'm still note quite sure if we really need the ImportFileFilter. Internally all it really does is new FileNameExtensionFilter(extensions.getDescription(), extensions.getExtensions()); and then overriding all methods introducing potential problems as in #1775.
There was a problem hiding this comment.
But ok, let's try this. Maybe we can reuse this then somehow.
There was a problem hiding this comment.
@Siedlerchr Moved the logic into the ImportFileFilter class.
There was a problem hiding this comment.
Yeah. That looks better. Keeps the dialog more clear. 👍
I have no further remarks. You can merge it,.
|
Generally a good solution, but I would move the logic to the ImportFileFilter, makes it a bit more clear. |
| @@ -303,7 +303,6 @@ | |||
| public static final String SHOW_FILE_LINKS_UPGRADE_WARNING = "showFileLinksUpgradeWarning"; | |||
| public static final String SIDE_PANE_WIDTH = "sidePaneWidth"; | |||
| public static final String LAST_USED_EXPORT = "lastUsedExport"; | |||
| public static final String LAST_USED_IMPORT = "lastUsedImport"; | |||
There was a problem hiding this comment.
Why did you remove the Preference? It is a really nice feature to preselect the last used Filefilter for exporting/importing.
There was a problem hiding this comment.
Because all supported filters are now default so you can definitely see possible files. If the importer has problems with auto recognition one can select the import format by hand (although this should not happen). Just wanted to remove complexity here. Maybe it's still needed - we have to discuss this...
There was a problem hiding this comment.
Yeah, you are right, forgot that new feature. Then it's ok to be removed.
2016-08-19 11:58 GMT+02:00 Stefan Kolb notifications@github.com:
In src/main/java/net/sf/jabref/preferences/JabRefPreferences.java
#1774 (comment):@@ -303,7 +303,6 @@
public static final String SHOW_FILE_LINKS_UPGRADE_WARNING = "showFileLinksUpgradeWarning";
public static final String SIDE_PANE_WIDTH = "sidePaneWidth";
public static final String LAST_USED_EXPORT = "lastUsedExport";
- public static final String LAST_USED_IMPORT = "lastUsedImport";
Because all supported filters are now default so you can definitely see
possible files. If the importer has problems with auto recognition one can
select the import format by hand (although this should not happen). Just
wanted to remove complexity here. Maybe it's still needed - we have to
discuss this...—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/JabRef/jabref/pull/1774/files/a3656a640c1ebb6b43447152a25fbed7b90a1a66#r75456162,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATi5COhrZNkN301wn-NogiV6caWxPP5ks5qhX44gaJpZM4Jnghh
.
# Conflicts: # src/main/java/net/sf/jabref/gui/importer/ImportFileFilter.java # src/main/java/net/sf/jabref/gui/importer/ImportFormats.java
Little bit tricky to get this done. Would have preferred a cleaner option but I still need access to some JFileChooser methods...