Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ExportFormats: Use FileExtension enum #3565

Merged
merged 6 commits into from
Dec 22, 2017
Merged

ExportFormats: Use FileExtension enum #3565

merged 6 commits into from
Dec 22, 2017

Conversation

PJozeph
Copy link
Contributor

@PJozeph PJozeph commented Dec 21, 2017 •
edited
Loading

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

tobiasdiez
tobiasdiez requested changes Dec 21, 2017
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you were able to solve your compile problems! Thanks for your contribution. The current code still needs a bit of work before we can merge it. See below for further details.

src/main/java/org/jabref/gui/exporter/ExportAction.java Outdated
@@ -82,7 +82,7 @@ public void actionPerformed(ActionEvent e) {

ExportFileFilter eff = (ExportFileFilter) ff;
String path = file.getPath();
if (!path.endsWith(eff.getExtension())) {
if (!path.endsWith(eff.getExtension().toString())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work: the FileExtensions.toString method will return the default java string serialization and not the extension stored internally. You should use FileExtensions.getExtensionsAsList() instead.

src/main/java/org/jabref/gui/exporter/ExportFileFilter.java Outdated
@@ -38,7 +39,7 @@ public boolean accept(File file) {
if (file.isDirectory()) {
return true;
} else {
return file.getPath().toLowerCase(Locale.ROOT).endsWith(extension);
return file.getPath().toLowerCase(Locale.ROOT).endsWith(extension.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same remark as above

src/main/java/org/jabref/logic/exporter/ExportFormat.java Outdated
@@ -78,7 +79,7 @@ public ExportFormat(String displayName, String consoleName, String lfFileName, S
*/
public ExportFormat(String displayName, String consoleName, String lfFileName, String directory, String extension,
LayoutFormatterPreferences layoutPreferences, SavePreferences savePreferences) {
this(displayName, consoleName, lfFileName, directory, extension);
this(displayName, consoleName, lfFileName, directory, FileExtensions.AUX);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you use ̀ AUXhere instead of the providedextension`?

src/main/java/org/jabref/logic/util/FileExtensions.java Outdated
@@ -42,7 +42,9 @@
CLASS(Localization.lang("%0 file", "CLASS"), "class"),
JAR(Localization.lang("%0 file", "JAR"), "jar"),
XML(Localization.lang("%0 file", "XML"), "xml"),
ZIP(Localization.lang("%0 file", "ZIP"), "zip");
ZIP(Localization.lang("%0 file", "ZIP"), "zip"),
ODS(Localization.lang("%0 file", "ZIP"), "ods"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct the description for ods and sxc.

@koppor koppor added this to the v4.1 milestone Dec 22, 2017
Siedlerchr
Siedlerchr reviewed Dec 22, 2017
View reviewed changes
src/main/java/org/jabref/logic/exporter/ExportFormats.java Outdated
*/
public static IExportFormat getExportFormat(String consoleName) {
return ExportFormats.EXPORT_FORMATS.get(consoleName);
}

public static FileExtensions getFileExtension(String consoleName) {
ExportFormat exportFormat = (ExportFormat) EXPORT_FORMATS.get(consoleName);
Copy link
Member

@Siedlerchr Siedlerchr Dec 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if no ExportFormat is not found for that console Name?
Then the exportFormat.getExtension will throw an NPE

Siedlerchr
Siedlerchr requested changes Dec 22, 2017
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to add a default/check for null in that case

LinusDietz
LinusDietz approved these changes Dec 22, 2017
View reviewed changes
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@LinusDietz LinusDietz merged commit d9644d0 into JabRef:master Dec 22, 2017
@tobiasdiez tobiasdiez mentioned this pull request Dec 22, 2017
Closed
@Siedlerchr Siedlerchr mentioned this pull request Dec 25, 2017
Closed
Siedlerchr added a commit that referenced this pull request Dec 28, 2017
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into sharelatex
c91fe49 
* upstream/master: (121 commits)
  Fix fetcher tests (#3583)
  Convert entry preview panel to JavaFX (#3574)
  Remove dependency for JUnit4 (#3580)
  Update tracis cache config as recommned by the docs (#3575)
  fix typo
  Show development information
  Release v4.1
  Add new authors to mailMap (#3567)
  Fix build
  Really fix changelog
  Fix changelog
  Minor changes
  Disable the autocompletion as default (#3569)
  Add update exception for applicationinsights 2.0.0-BETA (#3571)
  Install4j jres 152 (#3570)
  Fix grammar mistake
  Downgrade applicationinsights. Fixes #3561
  Adapt scripts/prepare-install4j.sh to JRE1.8.0_152 (#3568)
  ExportFormats: Use FileExtension enum (#3565)
  Update guava from 23.5-jre -> 23.6-jre
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez requested changes

@koppor koppor koppor approved these changes

@LinusDietz LinusDietz LinusDietz approved these changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v4.1
Development

Successfully merging this pull request may close these issues.
5 participants
@PJozeph @Siedlerchr @LinusDietz @koppor @tobiasdiez