Change some FileDialogs to DialogService by Siedlerchr · Pull Request #2767 · JabRef/jabref

Siedlerchr · 2017-04-19T13:40:28Z

Change nearly all occurences of FileDialog class to DialogService.
Internal change only.

The only piece I did/could not yet change is the piece in ImportFormats where the importer is selected based on the file filter.
That is a somehow ugly thing. @tobiasdiez Maybe you have an idea on how to solve this?

List<FileExtensions> extensions = importers.stream().map(Importer::getExtensions).collect(Collectors.toList());

....

 Optional<Importer> format = importers.stream().filter(i -> Objects.equals(i.getExtensions().getDescription(), dialog.getFileFilter().getDescription()))
                                .findFirst();

~~- [ ] Tests created for changes~~
~~- [ ] Change in CHANGELOG.md described~~
~~- [ ] 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?~~

Add method for selecting multiple files on opening TODO: Fix javadoc + change some other pieces

* upstream/master: Fix Drag and drop of file by first converting to File and then to path (#2764) Fix 'library' vs 'database' Chinese translation. (#2763)

…ormat

Siedlerchr · 2017-04-20T10:42:57Z

TODO: Find a way to change ImportFormat

tobiasdiez

In general looks good to me, I have more questions than actual critic.

Concerning the importer by extension business, I propose to use a map.
At some point you need to create ExtensionFilters from an importer. Then you can save this as a map ̀ Map<ExtensionFilter, Importer>`. Later you query the selectedExtensionFilterProperty to get the extension filter and the lookup your importer from this in the map. Does this sounds reasonable?

tobiasdiez · 2017-04-20T17:58:18Z

            selectionListener.setPreviewActive(true);
            showPreview(selectionListener.getPreview());
-            selectionListener.getPreview().getPrintAction().actionPerformed(new ActionEvent(this, ActionEvent.ACTION_PERFORMED, null));
+            selectionListener.getPreview().getPrintAction()


Just a general remark: is your IDE breaking all these lines? If I remember correctly, we said that we don't need a hard wrap at x characters because modern screens are relatively wide.

Is the default Eclipse setting, Maximum line width of 120 chars

Just disable it please.

Done. And I create a new PR so that it is included in the gradle command

tobiasdiez · 2017-04-20T18:10:00Z

-            for (File theFile : theFiles) {
-                frame.getFileHistory().newFile(theFile.getPath());
+            for (Path theFile : theFiles) {
+                frame.getFileHistory().newFile(theFile.toString());


I'm always confused by this: is Path.toString or Path.toAbsolutePath.toString the correct replacement for File.getPath?

I am not always sure, either. In theory toString should be enough. But sometimes it's necessary to call toAbsolutePath before, to really ensure that the file path is correct handled by the OS

tobiasdiez · 2017-04-20T18:11:12Z

-            File fileToLoad = file;
-            frame.output(Localization.lang("Opening") + ": '" + file.getPath() + "'");
+    private void openTheFile(Path file, boolean raisePanel) {
+        if ((file != null) && Files.exists(file)) {


I think it save to replace the null check with Objects.requireNonNull

tobiasdiez · 2017-04-20T18:12:04Z


-            String fileName = file.getPath();
-            Globals.prefs.put(JabRefPreferences.WORKING_DIRECTORY, file.getParent());
+            String fileName = file.getParent().toString();


̀ fileName` is a bit irritating name here since it is actually the directory name.

Changed that

tobiasdiez · 2017-04-20T18:13:08Z

-                JOptionPane.showMessageDialog(null, Localization.lang("Error opening file") + " '" + fileName + "'",
-                        Localization.lang("Error"), JOptionPane.ERROR_MESSAGE);
-            }
+            result = OpenDatabase.loadDatabase(fileToLoad.toAbsolutePath().toString(),


Introduce an overload of loadDatabase that accepts a path?

tobiasdiez · 2017-04-20T18:14:19Z

-        if (!fileToOpen.exists()) {
-            JOptionPane.showMessageDialog(frame, Localization.lang("File not found") + ": " + fileToOpen.getName(),
+        if (!Files.exists(fileToOpen)) {
+            JOptionPane.showMessageDialog(frame, Localization.lang("File not found") + ": " + fileToOpen.getParent(),


getParent or toString?

In this case I wanted to use getFileName

* upstream/master: Reimplement date editor in JavaFX (#2781) Update CONTRIBUTING.md Add new author Update Checkstyle Version fix some more checkstyle warnings fix some more checkstyle warnings Fix Build failure, hopefully Spanish translation (#2773) Fixes #2766 If file is not found annotations might be null Fix language tests Remove preferences and globals from tests (#2768) Fix Unable to create Checker Fix checkstyle warnings New checkstyle rules regarding spacing Reimplement owner editior in JavaFX Reimplement url editior in JavaFX Reimplement journal editior in JavaFX New checkstyle rules regarding spacing # Conflicts: # src/main/java/org/jabref/gui/importer/actions/AppendDatabaseAction.java # src/main/java/org/jabref/migrations/FileLinksUpgradeWarning.java

Make getConfiguredFilechooser public Fix unused variables Change some File usage to path

* upstream/master: Do not log an exception if side pane was not found (#2791) Added 'Ink' to the supported FileAnnotationType (required to close #2777) Renamed parseFileAnnotationType() to parse() Fixes handling of unknown PDAnnotation types.

* upstream/master: (84 commits) Update README.md Update CHANGELOG.md Fixes #2789 Add Referer to API call (#2794) Change some FileDialogs to DialogService (#2767) Fix for issue 2762: Change CSV export to separate all names using semicolon (#2793) Set eclipse line wrapping to maximum Do not log an exception if side pane was not found (#2791) Added 'Ink' to the supported FileAnnotationType (required to close #2777) Renamed parseFileAnnotationType() to parse() Reimplement date editor in JavaFX (#2781) Update CONTRIBUTING.md Add new author Fixes handling of unknown PDAnnotation types. Update Checkstyle Version fix some more checkstyle warnings fix some more checkstyle warnings Fix Build failure, hopefully Spanish translation (#2773) Fixes #2766 If file is not found annotations might be null Fix language tests ... # Conflicts: # src/main/java/org/jabref/logic/util/io/FileUtil.java

Siedlerchr added 6 commits April 19, 2017 15:40

Change some FileDialogs to DialogService

99d1e45

Add method for selecting multiple files on opening TODO: Fix javadoc + change some other pieces

Merge remote-tracking branch 'upstream/master' into fixFileDlgs

90ef1e3

* upstream/master: Fix Drag and drop of file by first converting to File and then to path (#2764) Fix 'library' vs 'database' Chinese translation. (#2763)

Changed some more dialogs and changed file to path

1cc80aa

Some more dialogs, add string overload for DirectoryDialog configurator

0171986

some more file dialogs

d2eae12

Changed rest of dialogs to use DialogService class except for importF…

b81455a

…ormat

Siedlerchr added 2 commits April 20, 2017 12:51

fix checkstyle

90ec742

Fix javadoc

ba8495d

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 20, 2017

tobiasdiez requested changes Apr 20, 2017

View reviewed changes

Siedlerchr added 5 commits April 23, 2017 18:19

Fix rest of file Dialogs, remove deprecated FileDialog class

7ac0294

Make getConfiguredFilechooser public Fix unused variables Change some File usage to path

remove empty lines fix checkstyle

1b3d27f

revert line wrapping of some long methods

5ae1dc4

Siedlerchr merged commit a403da5 into master Apr 26, 2017

Siedlerchr deleted the fixFileDlgs branch April 26, 2017 11:19

Siedlerchr mentioned this pull request Apr 29, 2017

NullPointerException in FileDialog #2756

Closed

Uh oh!

Conversation

Siedlerchr commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Siedlerchr commented Apr 19, 2017 •

edited

Loading

Siedlerchr commented Apr 20, 2017 •

edited

Loading