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

Make attached files relative to the file directory #4212

Merged
merged 7 commits into from
Jul 25, 2018
Merged

Make attached files relative to the file directory #4212

merged 7 commits into from
Jul 25, 2018

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Jul 17, 2018 •
edited
Loading

Fixes #4201

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@Siedlerchr
Make attached files relative to the file directory
482e7aa 
Fixes #4201
Open files with default OS application if no External file type is present
@Siedlerchr
add changelog
c32aca6
@Siedlerchr Siedlerchr changed the title Fixattachfile Make attached files relative to the file directory Jul 17, 2018
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 17, 2018
tobiasdiez
tobiasdiez requested changes Jul 17, 2018
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 would have preferred if the bug is fixed via refactoring. Other remarks are just minor nitpicking.

src/main/java/org/jabref/gui/filelist/AttachFileAction.java Outdated

dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(newFile -> {
LinkedFile newLinkedFile = new LinkedFile("", newFile.toString(), "");

Path relativePath = FileUtil.shortenFileName(newFile, panel.getBibDatabaseContext().getFileDirectoriesAsPaths(Globals.prefs.getFileDirectoryPreferences()));
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, this code should be refactored to use the same code as the same action in the entry editor. Otherwise, we will again run into a similar problem where the two actions are different in behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it

src/main/java/org/jabref/gui/filelist/LinkedFilesEditDialogViewModel.java Outdated
@@ -119,7 +119,7 @@ public StringProperty descriptionProperty() {
}

public LinkedFile getNewLinkedFile() {
return new LinkedFile(description.getValue(), link.getValue(), selectedExternalFileType.getValue().toString());
return new LinkedFile(description.getValue(), link.getValue(), selectedExternalFileType.getValue() == null ? null : selectedExternalFileType.getValue().toString());
Copy link
Member

Choose a reason for hiding this comment

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

As we wanted to get ride of null's, I don't like this change. Maybe we should use a MonadicBinding (optional binding) and use an empty string as file type?

src/main/java/org/jabref/gui/desktop/JabRefDesktop.java Outdated
@@ -160,6 +155,8 @@ private static void openExternalFilePlatformIndependent(Optional<ExternalFileTyp
} else {
NATIVE_DESKTOP.openFileWithApplication(filePath, application);
}
} else {
NATIVE_DESKTOP.openFile(filePath, null);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a short comment explaining what is done here.

@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixattachfile
cfa2365 
* upstream/master:
  Update dependencies (#4231)
  Update to latest release of richtextfx (#4213)
  Fix fetcher tests (#4216)
  single line text fields (#4138)
  Make it easier to rename and move files (#4200)
  Fix that swing dialogs are hidden behind main window (#4205)
  Fixed: #4166 add move & rename file (#4217)

# Conflicts:
#	CHANGELOG.md
@Siedlerchr
use method from linked files view model for linked file
a19125c
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixattachfile
971b3c7 
* upstream/master:
  Fix isbn chimbori test (#4234)
  Rewrite web search pane in JavaFX (#4203)
@Siedlerchr
fix opening of file from menu
4aba5a4 
add monadic binding
tobiasdiez
tobiasdiez approved these changes Jul 25, 2018
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.

Looks good to me now.

@Siedlerchr Siedlerchr merged commit 077fdac into master Jul 25, 2018
@Siedlerchr Siedlerchr deleted the fixattachfile branch July 25, 2018 14:50
Siedlerchr added a commit that referenced this pull request Jul 26, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixdragandrop
1d4db3d 
* upstream/master: (47 commits)
  Make attached files relative to the file directory (#4212)
  execute set visible in swing thread to avoid blocking
  Fix isbn chimbori test (#4234)
  Rewrite web search pane in JavaFX (#4203)
  Update dependencies (#4231)
  Update to latest release of richtextfx (#4213)
  Fix fetcher tests (#4216)
  single line text fields (#4138)
  Make it easier to rename and move files (#4200)
  Fix that swing dialogs are hidden behind main window (#4205)
  Fixed: #4166 add move & rename file (#4217)
  update gradle plugins and gradlen to 4.9
  Update dependencies
  Remove unnecessary look and feel migration (#4204)
  Revert threading fix since this sometimes lead to freezes...
  Update year
  Fix a few more threading exceptions
  Refactor
  Convert CiteSeerX fetcher to new infrastructure (#4185)
  Make global font size customizable (#4178)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/BasePanel.java
#	src/main/java/org/jabref/gui/JabRefFrame.java
#	src/main/java/org/jabref/gui/mergeentries/MergeEntries.java
#	src/main/resources/l10n/JabRef_en.properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Siedlerchr @tobiasdiez