-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change some FileDialogs to DialogService #2767
Conversation
Add method for selecting multiple files on opening TODO: Fix javadoc + change some other pieces
TODO: Find a way to change ImportFormat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -2367,7 +2385,8 @@ public void action() { | |||
public void action() throws Exception { | |||
selectionListener.setPreviewActive(true); | |||
showPreview(selectionListener.getPreview()); | |||
selectionListener.getPreview().getPrintAction().actionPerformed(new ActionEvent(this, ActionEvent.ACTION_PERFORMED, null)); | |||
selectionListener.getPreview().getPrintAction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the default Eclipse setting, Maximum line width of 120 chars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just disable it please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. And I create a new PR so that it is included in the gradle command
for (File theFile : theFiles) { | ||
frame.getFileHistory().newFile(theFile.getPath()); | ||
for (Path theFile : theFiles) { | ||
frame.getFileHistory().newFile(theFile.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always confused by this: is Path.toString
or Path.toAbsolutePath.toString
the correct replacement for File.getPath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
File fileToLoad = file; | ||
frame.output(Localization.lang("Opening") + ": '" + file.getPath() + "'"); | ||
private void openTheFile(Path file, boolean raisePanel) { | ||
if ((file != null) && Files.exists(file)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it save to replace the null
check with Objects.requireNonNull
|
||
String fileName = file.getPath(); | ||
Globals.prefs.put(JabRefPreferences.WORKING_DIRECTORY, file.getParent()); | ||
String fileName = file.getParent().toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
̀ fileName` is a bit irritating name here since it is actually the directory name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed that
JOptionPane.showMessageDialog(null, Localization.lang("Error opening file") + " '" + fileName + "'", | ||
Localization.lang("Error"), JOptionPane.ERROR_MESSAGE); | ||
} | ||
result = OpenDatabase.loadDatabase(fileToLoad.toAbsolutePath().toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce an overload of loadDatabase
that accepts a path?
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getParent
or toString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: (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
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?
- [ ] Tests created for changes- [ ] Change in CHANGELOG.md described- [ ] Screenshots added (for bigger UI changes)- [ ] Check documentation status (Issue created for outdated help page at help.jabref.org?)- [ ] If you changed the localization: Did you rungradle localizationUpdate
?