Refactor Open and Save Dialogs #1336

Siedlerchr · 2016-05-01T18:24:26Z

Complete Refactoring of the ugly and propably outdated FileDialogs code
NewFileDialogs will be renamed when the work is done
Atm I want to keep it parallel.

TODO:

Refactor getMultpleFiles
Provide methods for other Open and Save Dialogs
Replace OpenFileFilter with FileNameExtensionFilter http://docs.oracle.com/javase/8/docs/api/javax/swing/filechooser/FileNameExtensionFilter.html
Refactor BrowseAction to use new the Methods
Test "Main file directory" should be relative to the bib file #1324 again

tobiasdiez · 2016-05-01T21:15:55Z

Good idea to refactor the file dialogs 👍. Just one small comment so far: could you please make the methods non-static (and probably move the passed frame to the constructor). In this way the code becomes more testable because the file dialog can be mocked away.

Siedlerchr · 2016-05-02T07:47:05Z

@tobiasdiez Yes, I already came up with a solution for this. I will probably use some kind of FactorPattern to create the dialogs. That reduces the number of constructors/method overloads.

codecov-io · 2016-05-02T19:47:20Z

Current coverage is 27.54%

Merging #1336 into master will increase coverage by +<.01%

@@             master      #1336   diff @@
==========================================
  Files           694        696     +2   
  Lines         46262      46335    +73   
  Methods           0          0          
  Messages          0          0          
  Branches       7649       7655     +6   
==========================================
+ Hits          12733      12759    +26   
- Misses        32432      32476    +44   
- Partials       1097       1100     +3

2 files (not in diff) in ...va/net/sf/jabref/gui were modified. more
- Misses -2
- Partials +3
- Hits +22
1 files (not in diff) in ...n/java/net/sf/jabref were modified. more
- Hits +4
File ...ClipboardAction.java (not in diff) was created. more

Powered by Codecov. Last updated by 71eca69...5371fd4

stefan-kolb · 2016-07-12T13:58:41Z

What is the status here? I'd love to see these changes inside JabRef 3.5!

Siedlerchr · 2016-07-12T14:04:55Z

Unfortunately I have not that much time atm. But I will try to see how far I get the next days. Mabye at the weekend I can get some time for it. Atm another project ist due on sunday...;)

Siedlerchr · 2016-07-24T16:57:56Z

Got some time working on it.

oscargus · 2016-07-30T18:29:09Z

src/main/java/net/sf/jabref/gui/FileExtensions.java

+        this.description = description;
+    }
+
+    public String[] getExtensions() {


Normally I would agree with you, but in this case an Array is better, because a) varargs (...) as parameter is handled as array and b) the array is then passed again as varags parameter to the FileNameFilterExtension.
A list would have to be converted back to array again to be passed as paramter.
And the decison for varargs was to support multiple file extension for one type in the enum e.g. Import_Format("Sample input format",xml,bib,dat)
http://docs.oracle.com/javase/1.5.0/docs/guide/language/varargs.html

OK! Makes sense!

NewFileDialogs will be renamed when the work is done Reworked BrowseAction, added Enum for file extensions Use a more factory like pattern to create fileDialogs Avoids unnecessary constructor overloads Reworked FileExtensions to accept mutiple Extensions for one description Added OverwriteExistingFile in JFileChooser Refactored rest of the dialogs Removed obsolte keys Remove comment

Siedlerchr · 2016-08-06T17:14:36Z

Ready for review.

oscargus · 2016-08-07T13:24:48Z

src/main/java/net/sf/jabref/gui/FileListEntryEditor.java

@@ -199,6 +202,8 @@ public void changedUpdate(DocumentEvent documentEvent) {

        });



Get rid of print-debugging. :-)

tobiasdiez · 2016-08-07T14:43:15Z

src/main/java/net/sf/jabref/gui/NewFileDialogs.java

+        return fileChooser.showDialog(parent, null) == JFileChooser.APPROVE_OPTION;
+    }
+
+    private static String getWorkingDir() {


Accept JabRefPreferences instead of operating on Globals.prefs

Not sure, but you mean sth like this:
JabRefPreferences.getInstance().get(JabRefPreferences.WORKING_DIRECTORY);

This is gui code so no need to inject JabRefPreferences? (I think @tobiasdiez means private static String getWorkingDir(JabRefPreferences prefs) {)

Yes, all gui code, but still a bit confused about the Pref stuff 😕
The getWorkingDir() is only called from the constructor.

public NewFileDialogs(JFrame owner) { this(owner, getWorkingDir()); }

So here I would need to pass the JabRefPreferences.getInstance()?

Yes I thought about injecting the JabRefPreferences class. So
̀getWorkingDir(JabRefPreferences preferences) and NewFileDialogs(JFrame owner, JabRefPreferences preferences) which then is called via new NewFileDialogs(owner, Globals.prefs).

Just because its GUI code doesn't mean that it is ok to work on Globals.prefs. We are just not as strict in enforcing the rule since sometimes it is very hard to implement. But here it is relatively simple.

I agree that it is simple here, but I'm still under the impression that it indeed is OK. With that said, I also happens to inject it in gui code every now and then.

I guess here one may actually inject the preference value rather than the complete preferences as well?

Oh, injecting the correct value in this particular case wouldn't make much sense. :-)

I thought about this again and I see no advantages over injecting the Preferences or passing everytime the working directory.
The goal of the redesign was that the developer does not have to care about setting/passing the working directory every time. Passing the Preferences every time would not make any sense to me. Addtionally, it would create some extra dependencies of Globals.prefs to the caller class.

tobiasdiez · 2016-08-07T14:48:11Z

I really like the new FileDialog class and its fluent interface. The code needs some cleanup in general (remove commented-out code and debug prints, no abbreviations, ...) and then I think it is good to go.

tobiasdiez · 2016-08-07T14:50:02Z

src/main/java/net/sf/jabref/gui/FileExtensions.java

+    TXT(
+            "Plain text file", "txt"),
+    CLASS(
+            "Class file", "class"),


I think most of the description values should actually be localized.

I was not sure about this, so I wanted to wait for feedback. But I agree that it probably makes sense.

Use optional Use Localization

Siedlerchr · 2016-08-07T17:27:56Z

Adressed comments. @tobiasdiez Not sure if I understand you right on how I should handle the WorkingDir-Prefs-stuff.
To me it does not make sense to pass the Working dir to the constructor from an external call. It would create unnecessary dependencies to the prefs. Or what was your idea?

Some renaming

koppor · 2016-08-09T12:34:35Z

src/main/java/net/sf/jabref/gui/FileExtensions.java

+    RIS(
+            Localization.lang("%0 file", "RIS"), "ris"),
+    ENDNOTE(
+            Localization.lang("Endnote/Refer file"), "ref"),


Please rewrite to Localization.lang("%0 file", "Endnote/Refer"), "ref"),

* master: (22 commits) Do not cite entries without a key in OpenOffice/LibreOffice (JabRef#1682) (JabRef#1683) Got rid of unused preferences (JabRef#1672) Move labelpattern (JabRef#1626) Implementation of shared database support (full system). (JabRef#1451) Removed bst from architecture tests JabRef#1699 Update PULL_REQUEST_TEMPLATE.md French localization: Menu: Translation of an empty string French localization: Jabref_fr: empty strings translated Updated string similarity version (JabRef#1698) Minify description of release process Announce switch from GPL to MIT in CONTRIBUTING.md and README.md Some cleanups to initialize empty MetaData at fewer positions (JabRef#1696) Automatic group names are converted from LaTeX to Unicode (JabRef#1684) Update ISSUE_TEMPLATE.md (JabRef#1686) Removed (false) NPE issue reported by FindBugs More Swedish translations (JabRef#1691) Updated wiremock version (JabRef#1690) Some minor code cleanups (JabRef#1689) Removed dependencies of Globals.prefs in some tests (JabRef#1688) Lookup BibEntry from ISBN and merge information (JabRef#1621) ... # Conflicts: # src/main/java/net/sf/jabref/gui/BasePanel.java # src/main/java/net/sf/jabref/importer/ImportMenuItem.java # src/main/resources/l10n/JabRef_da.properties # src/main/resources/l10n/JabRef_de.properties # src/main/resources/l10n/JabRef_en.properties # src/main/resources/l10n/JabRef_es.properties # src/main/resources/l10n/JabRef_fa.properties # src/main/resources/l10n/JabRef_fr.properties # src/main/resources/l10n/JabRef_in.properties # src/main/resources/l10n/JabRef_it.properties # src/main/resources/l10n/JabRef_ja.properties # src/main/resources/l10n/JabRef_nl.properties # src/main/resources/l10n/JabRef_no.properties # src/main/resources/l10n/JabRef_pt_BR.properties # src/main/resources/l10n/JabRef_ru.properties # src/main/resources/l10n/JabRef_sv.properties # src/main/resources/l10n/JabRef_tr.properties # src/main/resources/l10n/JabRef_vi.properties # src/main/resources/l10n/JabRef_zh.properties

Fix merge error Fix codacy issues

Siedlerchr · 2016-08-09T20:05:21Z

@oscargus Could you please have a look why the cleanupCasesAddsBracketAroundAluminiumGalliumArsenid test is failing?
Locally all is okay for me. I have no idea what it could cause on Travis. And I didn't touch any of the cleanup code in this PR . 😕

Fixed typo

Refactored Open and Save dialogs Introduced FileExtensions enum for FileDialog Filter Updated BrowseAction to use new dialogs Fix JabRef#1324 Fix JabRef#1609

This was referenced May 20, 2016

Importer open dialog improvements #1431

Closed

Better default for saving files #1446

Closed

Siedlerchr mentioned this pull request Jul 21, 2016

"New file link" opens wrong folder in FileDialog #1609

Closed

Siedlerchr added the type: enhancement label Jul 24, 2016

Siedlerchr assigned Siedlerchr and unassigned Siedlerchr Jul 24, 2016

Siedlerchr mentioned this pull request Jul 27, 2016

Replaced some getField and fixed some bugs #1631

Merged

1 task

oscargus reviewed Jul 30, 2016
View reviewed changes

This was referenced Jul 31, 2016

Removal of layout-extension field in CustomExport #1653

Closed

Fixed one more NPE which should have been fixed in #1631 #1649

Merged

Siedlerchr force-pushed the fileChooserRefactor branch from 003d4e4 to 8453c2d Compare August 6, 2016 16:58

Siedlerchr force-pushed the fileChooserRefactor branch from 8453c2d to 39dbf77 Compare August 6, 2016 17:05

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

oscargus reviewed Aug 7, 2016
View reviewed changes

tobiasdiez reviewed Aug 7, 2016
View reviewed changes

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 7, 2016

tobiasdiez reviewed Aug 7, 2016
View reviewed changes

Remove debug code

d84191d

Use optional Use Localization

Add javadoc

fe8a9a4

Some renaming

koppor reviewed Aug 9, 2016
View reviewed changes

Siedlerchr mentioned this pull request Aug 9, 2016

"Main file directory" should be relative to the bib file #1324

Closed

Siedlerchr added 2 commits August 9, 2016 21:15

Update localization

669a3a1

Fix merge error Fix codacy issues

Fix failing test by adding trim

f368f78

Siedlerchr added 2 commits August 10, 2016 17:48

Add changelog

09f072e

Fixed typo

Rebase

2b63014

Siedlerchr merged commit 8342956 into JabRef:master Aug 10, 2016

Siedlerchr deleted the fileChooserRefactor branch August 10, 2016 16:36

This was referenced Aug 10, 2016

Browse for .aux file does not take current folder as starting folder koppor/jabref#153

Closed

Windows version uses old-school explorer dialog when opening files koppor/jabref#154

Closed

Support Native filedialogs (JavaFX only) #1711

Closed

koppor changed the title ~~WIP: Refactor Open and Save Dialogs~~ Refactor Open and Save Dialogs Aug 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Open and Save Dialogs #1336

Refactor Open and Save Dialogs #1336

Siedlerchr commented May 1, 2016 •

edited

Loading

tobiasdiez commented May 1, 2016

Siedlerchr commented May 2, 2016

codecov-io commented May 2, 2016 •

edited

Loading

stefan-kolb commented Jul 12, 2016

Siedlerchr commented Jul 12, 2016

Siedlerchr commented Jul 24, 2016

oscargus Jul 30, 2016

Siedlerchr Jul 30, 2016

oscargus Jul 30, 2016 via email

Siedlerchr commented Aug 6, 2016

oscargus Aug 7, 2016

tobiasdiez Aug 7, 2016

Siedlerchr Aug 7, 2016

oscargus Aug 7, 2016

Siedlerchr Aug 7, 2016

tobiasdiez Aug 8, 2016

oscargus Aug 8, 2016

oscargus Aug 8, 2016

Siedlerchr Aug 8, 2016

tobiasdiez commented Aug 7, 2016

tobiasdiez Aug 7, 2016

Siedlerchr Aug 7, 2016

Siedlerchr commented Aug 7, 2016

koppor Aug 9, 2016

Siedlerchr commented Aug 9, 2016

		@@ -199,6 +202,8 @@ public void changedUpdate(DocumentEvent documentEvent) {

		});

Refactor Open and Save Dialogs #1336

Refactor Open and Save Dialogs #1336

Conversation

Siedlerchr commented May 1, 2016 • edited Loading

tobiasdiez commented May 1, 2016

Siedlerchr commented May 2, 2016

codecov-io commented May 2, 2016 • edited Loading

Current coverage is 27.54%

stefan-kolb commented Jul 12, 2016

Siedlerchr commented Jul 12, 2016

Siedlerchr commented Jul 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus Jul 30, 2016 via email

Choose a reason for hiding this comment

Siedlerchr commented Aug 6, 2016

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

tobiasdiez commented Aug 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Aug 7, 2016

Choose a reason for hiding this comment

Siedlerchr commented Aug 9, 2016

Siedlerchr commented May 1, 2016 •

edited

Loading

codecov-io commented May 2, 2016 •

edited

Loading