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

Add copy linked files action to contextmneu in general tab #5068

Merged
merged 5 commits into from
Jul 6, 2019
Merged

Add copy linked files action to contextmneu in general tab #5068

merged 5 commits into from
Jul 6, 2019

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Jun 20, 2019 •
edited
Loading

Fixes #5066

  • 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 Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 20, 2019
tobiasdiez
tobiasdiez requested changes Jun 21, 2019
View reviewed changes
src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java
@@ -48,10 +52,14 @@
@FXML private final LinkedFilesEditorViewModel viewModel;
@FXML private ListView<LinkedFileViewModel> listView;

@Inject private StateManager stateManager;
private final DialogService dialogService;
Copy link
Member

Choose a reason for hiding this comment

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

Use @Inject ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually tried to inject the other stuff as well, but the key problem is that the fxml loader requires the viewModel to be intiialized beforehand cause in the fxml there is an expression on the viewModel defined. And in the ctor the Injects stuff is of course still null.

src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java Outdated Show resolved Hide resolved
@Siedlerchr
Add option to copy single file only
6f20c85 
Refactor preferences parameter
Remove unsused parameter from dialog
@Siedlerchr
fix checkstyle
48087ed
@Siedlerchr
Copy link
Member Author

Added a variant for a single entry and did some refactoring.

tobiasdiez
tobiasdiez requested changes Jun 29, 2019
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. Just some minor remarks

src/main/java/org/jabref/gui/copyfiles/CopySingleFileAction.java
.build();
Optional<Path> exportPath = dialogService.showDirectorySelectionDialog(dirDialogConfiguration);
exportPath.ifPresent(this::copyFileToDestination);

Copy link
Member

Choose a reason for hiding this comment

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

remove empty new line

src/main/java/org/jabref/gui/copyfiles/CopySingleFileAction.java
private void copyFileToDestination(Path exportPath) {
Optional<Path> fileToExport = linkedFile.findIn(databaseContext, Globals.prefs.getFilePreferences());

Optional<Path> newPath = OptionalUtil.combine(Optional.of(exportPath), fileToExport, resolvePathFilename);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit overly complicated as we can only copy the file if we find it (e.g. fileToExport is not empty). Moreover, please show a warning in case it is empty.

src/main/java/org/jabref/gui/externalfiles/FindFullTextAction.java Outdated
@@ -137,7 +137,7 @@ private void addLinkedFileFromURL(URL url, BibEntry entry, Path targetDirectory)
basePanel.getBibDatabaseContext(),
Globals.TASK_EXECUTOR,
dialogService,
JabRefPreferences.getInstance(), ExternalFileTypes.getInstance());
JabRefPreferences.getInstance().getXMPPreferences(),JabRefPreferences.getInstance().getFilePreferences(), ExternalFileTypes.getInstance());
Copy link
Member

Choose a reason for hiding this comment

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

please put them on new lines as the other dependencies

src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java Outdated Show resolved Hide resolved
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into copyLinkedFilesIn…
0c027b0 
…Contextmenu

* upstream/master: (21 commits)
  Bump junit-vintage-engine from 5.4.2 to 5.5.0
  Bump log4j-api from 2.11.2 to 2.12.0
  Bump junit-platform-launcher from 1.4.2 to 1.5.0
  Bump log4j-core from 2.11.2 to 2.12.0
  Bump log4j-slf4j18-impl from 2.11.2 to 2.12.0
  Bump log4j-jul from 2.11.2 to 2.12.0
  Bump junit-jupiter from 5.4.2 to 5.5.0
  Bump com.github.johnrengelman.shadow from 5.0.0 to 5.1.0
  Bump log4j-jcl from 2.11.2 to 2.12.0
  Bump fontbox from 2.0.15 to 2.0.16 (#5080)
  Bump pdfbox from 2.0.15 to 2.0.16 (#5081)
  Bump xmpbox from 2.0.15 to 2.0.16 (#5082)
  Update FUNDING.yml
  Delete autoapproval.yml
  Bump com.install4j.gradle from 7.0.11 to 7.0.12 (#5077)
  Configure autoapproval plugin for dependabot
  Bump xmlunit-matchers from 2.6.2 to 2.6.3 (#5074)
  Bump checkstyle from 8.21 to 8.22 (#5072)
  Bump xmlunit-core from 2.6.2 to 2.6.3 (#5073)
  Refactor for loop to stream in groupTree Close version socket
  ...
@Siedlerchr
rename variables, add message if file not resolved
c2cacfe
@Siedlerchr Siedlerchr merged commit add35be into master Jul 6, 2019
@Siedlerchr Siedlerchr deleted the copyLinkedFilesInContextmenu branch July 6, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez requested 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.

Entry editor: "Copy linked files"
2 participants
@Siedlerchr @tobiasdiez