Export pdf/linked files by Siedlerchr · Pull Request #3147 · JabRef/jabref

Siedlerchr · 2017-08-22T17:03:55Z

Followup from #2706 because somehow my new changes did not update the PR correctly.

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?

lenhard

I've tested it locally and it works in principle.

There's one thing I would like you to add, though: logging: When I do the export, I would like to have a log statement that says where I exported to, or alternatively if I tried to export and it failed. This will make error diagnosis later much easier. And you can expect to get issue reports, because this is something that works with the file system, so there are users who will use it in weird circumstances.

lenhard · 2017-08-27T16:17:33Z

+
+public class ExportLinkedFilesAction extends AbstractAction {
+
+    private final BiFunction<Path, Path, Path> resolvePathFilename = (p, f) -> {


Please use meaningful names (not p, f) even in lambdas

lenhard · 2017-08-27T16:17:47Z

+    private final BiFunction<Path, Path, Path> resolvePathFilename = (p, f) -> {
+        return p.resolve(f.getFileName());
+    };
+    private final DialogService ds = new FXDialogService();


Same here, better name please.

lenhard · 2017-08-27T16:19:55Z

+
+        long totalFilesCount = entries.stream().flatMap(entry -> entry.getFiles().stream()).count();
+
+        Service<Void> service = new Service<Void>() {


The enclosing actionPerformed() is a really big method. Please do some refactoring and make it smaller. The first thing would be to extract this class, at least to an inner class instead of an anonymous one.

lenhard · 2017-08-27T16:20:25Z

+        service.start();
+
+        DefaultTaskExecutor.runInJavaFXThread(() -> {
+            ProgressDialog dlg = new ProgressDialog(service);


And again, more meaningful names please :-)

koppor · 2017-08-31T06:00:42Z

Is it possible to add this to the context menu? I once added "Send as email" to the menu as I also needed a similar functionality for sending entries as email:

koppor · 2017-08-31T06:03:00Z

@Siedlerchr Your PR did not update as you named the branches differently: exportPdf vs. exportPDF. The letters D and F are cased differently. Thus, these are different branches. 🎉

koppor · 2017-08-31T06:10:42Z

Current accessibility:

Reading #2706 (comment) and the following comments, I do not like the word "Export" in the Tools menu. It took me 5 minutes to find the functionality.

Context menu: No entry for that
File menu: No entry for that
File -> Export: Cannot select "PDFs" somehow.

Suggestion:

Rename to "Copy attached files to folder..."

Further minor issues

progress always shows "exporting file 0 of x"
Last chosen folder is not remembered. The browse-dialog always starts in %HOME
Pressing "Cancel" when choosing the folder also triggers the export.

LinusDietz · 2017-08-31T06:51:48Z

Added devcall label to discuss milestone

koppor · 2017-08-31T12:05:39Z

Stays at v4.1 milestone as v4.1 will be our PDF feature release.

Remove code that produces NPE

Remove code that produces NPE Export linked files Don't replace existings Create new Action for exporting files TODO: Add to menu Move to Tools entry try around with some background task stuf remove compile errro Add Progessbar dialog Add localization Fix checkstyle extract to inner class Add some more output messages renamed variables Fix gui display

Fix localization to copy files

Siedlerchr · 2017-10-07T14:16:22Z

I think I fixed all issues raised by @koppor and I also create a log file in the export dir.

lenhard

Overall, the code looks fine. I just have a few suggestions for quality improvements. It would be cool if you could have a go at these, but it wouldn't block this PR if you don't.

The only real blocker is the missing changelog entry :)

lenhard · 2017-10-07T16:14:32Z

    public void start(Stage mainStage) throws Exception {
        Platform.setImplicitExit(false);
        SwingUtilities.invokeLater(() -> start(arguments));
+


Why is there a blank line here?

lenhard · 2017-10-07T16:18:08Z

+        });
+    }
+
+    private class ExportService extends Service<Void> {


This is rather nitpicky, but what about extracting this into a separate class file in the same package with default visibility? You could also extract parts of call() into separate private methods. All this wouldn't change anything about the functionality, but it would make the code a little less chunky and more readable.

I see no reason why I should extract that in a new class. It's just a single Service for one purpose.
But I think I can surely extract some private methods

One reason is that you could (and should) write tests for the ExportService.

Avoid code duplicaiton

tobiasdiez · 2017-10-08T04:39:20Z

+        DefaultTaskExecutor.runInJavaFXThread(() -> {
+            service.start();
+
+            ProgressDialog progressDialog = new ProgressDialog(service);


Can you please extract the "show progress dialog" part of this method to the DialogService class.

tobiasdiez · 2017-10-08T04:43:22Z

+                            List<LinkedFile> files = entries.get(i).getFiles();
+
+                            for (int j = 0; j < files.size(); j++) {
+                                updateMessage(Localization.lang("Copying file %0 of BibEntry %1", Integer.toString(j + 1), Integer.toString(i + 1)));


I think we never used BibEntry before in the user interface (it is the class name...). What about only "entry" or "item"?

tobiasdiez · 2017-10-08T04:50:12Z

+
+                            for (int j = 0; j < files.size(); j++) {
+                                updateMessage(Localization.lang("Copying file %0 of BibEntry %1", Integer.toString(j + 1), Integer.toString(i + 1)));
+                                Thread.sleep(500); //TODO: Adjust/leave/any other idea?


This sleep makes the whole progress of copying files really slow. I would remove it even through then the user has no chance to actually read the message for small files (but that is ok in my opinion).

tobiasdiez · 2017-10-08T04:52:36Z

+                                Thread.sleep(500); //TODO: Adjust/leave/any other idea?
+
+                                String fileName = files.get(j).getLink();
+                                Optional<Path> fileToExport = FileHelper.expandFilenameAsPath(fileName,


Use

jabref/src/main/java/org/jabref/model/entry/LinkedFile.java

Line 131 in fae290c

public Optional<Path> findIn(BibDatabaseContext databaseContext, FileDirectoryPreferences fileDirectoryPreferences) {

tobiasdiez · 2017-10-08T04:56:48Z

+                    updateMessage(Localization.lang("Copying files..."));
+                    updateProgress(0, totalFilesCount);
+
+                    try (BufferedWriter bw = Files.newBufferedWriter(exportPath.get().resolve(LOGFILE), StandardCharsets.UTF_8)) {


In my opinion, a log file is an inconvenient solution for the user. Why don't log all the errors to an internal list and display this list at the end in a new dialog?

tobiasdiez · 2017-10-08T04:59:26Z

    }

-    @Override
-    public void performExport(final BibDatabaseContext databaseContext, Path file, final Charset encoding,


Please don't delete this path based methods. I know they are not yet used but they are actually preferable to the string or file-based methods (these should actually be deprecated).

I deleted it on purpose because it is not working at all as I explained in my other PR or earlier.
It always lead to an exception.

tobiasdiez · 2017-10-08T05:00:35Z


        add(new GeneralAction(Actions.SEND_AS_EMAIL, Localization.lang("Send as email"), IconTheme.JabRefIcon.EMAIL.getSmallIcon()));
        addSeparator();
+        add(new ExportLinkedFilesAction());


Do you really think that this export functionality is used that often that we should add it to the right click menu?

It was requested by @koppor And I think it makes sense to have it in the context menu

* upstream/master: Set WM_CLASS to org-jabref-JabRefMain (#3273) Terminate JavaFX thread when running with "nogui" option (#3274)

* upstream/master: Update gradle to 4.2.1 (#3322) Avoid recreation of the EntryEditor (#3187) Fix #3133 telemetry by locking azure to 1.0.9 German translation for missing properties (#3312) Improvement for Java FX font rendering on Linux (#3305) Add also conversion for em dash Add \textendash to the html conversion table Update latex2tunicode from 0.2.1 -> 0.2.2 Added note about updating controlsfx Fix exception/freezing on EntryChangedEvent in Entry Editor (#3299) Update libs (#3300) update guava from 23.0 -> 23.2 update mvvmfx-validation from 1.6.0 -> 1.7.0 Resolves #3255 file open dialog should have "supported formates" filetype Fix #3235: remote metadata is updated instead of delete + insert (#3282) Change OO paths to Libre Office in preferences (#3287) Fix #2471: remove line breaks from abstracts in ADS fetcher (#3285) Resolves #3280 Empty String instead of N/A (#3288)

tobiasdiez · 2017-10-21T04:13:43Z

+            </TableView>
+            <ButtonBar prefHeight="40.0" prefWidth="200.0">
+               <buttons>
+                  <Button defaultButton="true" mnemonicParsing="false" onAction="#close" text="OK" />


you should always annotate buttons in a button bar with a proper "button data", which lets JavaFX position these buttons appropriately according to the style guidelines of the current os.

* upstream/master: Change integrity message for names depending on database mode (#3330) Fix location bundle with fast access (#3327) Small code cleanup Fix "path not found" exception # Conflicts: # src/main/java/org/jabref/logic/l10n/Localization.java

Siedlerchr · 2017-10-21T11:35:01Z

+import java.util.ArrayList;
+import java.util.List;
+
+public class CopyFilesResultListDependency {


@tobiasdiez I have created this wrapper class to inject it

If I dont' intilaize the ArrayList with Empty, it will give an NPE in my View Model which leads to the conclusion that this are somehow different objects. I remember having similair problems wit the Sharelatex Integration and ended up making it static. But maybe there is another solution?

Oh damn...thats ugly. It is not possible to directly inject the list of results?
You also hit this bug here: AdamBien/afterburner.fx#71 this is why you need the no-arg constructor.

I'll have a look at it later.

The list can not be directly injected, because List is an interface which of course can not be instantiated...that's where our default injector throws an error
I tried to add handling for list but then generics are a problem...

@tobiasdiez I found the issue. The "key" of the Hashmap and the name of the with inject annotated variable differed! That's why the data was not passed correct

With #3334 you should be able to use @Inject List<whatever> toBeInjected or other complex types (they do not need to have a no-args constructor anymore). Of course the names of the "key" and the "field" have to coincide to make this work.

Siedlerchr · 2017-10-21T11:35:47Z

    private void showDialog(List<CopyFilesResultItemViewModel> data) {
-        Dialog<ButtonType> dlg = new Dialog<>();
+
+        CopyFilesDialogView dlg = new CopyFilesDialogView(databaseContext, new CopyFilesResultListDependency(data));


@tobiasdiez Here I am passing the instance of the object, but it doesn't display any data.

Switch gui to border pane add tooltip to factory

* upstream/master: Initializing EntryEditor Tabs on focus (#3331) Fix #3292: annotations are now automatically refreshed (#3325) # Conflicts: # CHANGELOG.md

* upstream/master: Fix NPE when calling with bib file as cmd argument (#3343) update mockito-core from 2.10.0 -> 2.11.0 (#3338) Remove underscore in Localized messages (#3336) Localisation: French: new entries translated (#3337) Refactoring: Lazy init of all editor tabs (#3333)

LinusDietz · 2017-10-25T16:30:20Z

The result dialog is displayed way too small on my display.

When I try to resize it, a magical fourth column appears.

LinusDietz

Just some minor remarks which are not really blockers.

LinusDietz · 2017-10-25T16:38:38Z

+public class CopyFilesTask extends Task<List<CopyFilesResultItemViewModel>> {
+
+    private static final Log LOGGER = LogFactory.getLog(ExportLinkedFilesAction.class);
+    private static final String LOGFILE = "exportLog.log";


I am unsure if this logfile is needed.

If it is, I would encode a timestamp into the filename to be able to distinguish several exports into the same folder.

LinusDietz · 2017-10-25T16:39:54Z

+    private List<BibEntry> entries;
+
+    public ExportLinkedFilesAction() {
+        super(Localization.lang("Copy attached files to folder..."));


The terminology should be consistent. I suggest "Copy linked files to folder.."

LinusDietz · 2017-10-25T16:44:45Z

+        colMessage.setCellValueFactory(cellData -> cellData.getValue().getMessage());
+        colStatus.setCellValueFactory(cellData -> cellData.getValue().getIcon());
+
+        colFile.setCellFactory(new ValueTableCellFactory<CopyFilesResultItemViewModel, String>().withText(item -> item).withTooltip(item -> item));


is there a way to prevent that fourth column appearing #3147 (comment) ?

Add timestamp Improve GUI

Siedlerchr · 2017-10-28T18:28:41Z

@lynyus I changed the column size and there is also a tooltip for the file column

* upstream/master: Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Remove unnecessary EntrySorter Move existing code to gui and rename change classes to view model Small code improvements

…ortPdf * 'exportPdf' of https://github.com/JabRef/jabref: Rename l10n Add timestamp Improve GUI

lenhard

I had another look at the code, which seems fine. I think you have addressed all the other reviewers comments, so I"ll merge this now.

* upstream/master: (22 commits) Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Delete unused code Extract difference finder from gui to logic Remove unnecessary EntrySorter ... # Conflicts: # src/main/java/org/jabref/gui/entryeditor/SourceTab.java

* upstream/master: (26 commits) Fix static localized text (#3400) Fix problems in source editor (#3398) Fix MathSciNet fetcher (#3402) Fix NPE when saving new file Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle ... # Conflicts: # CHANGELOG.md

* upstream/master: Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388)

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

lenhard reviewed Aug 27, 2017

View reviewed changes

LinusDietz added this to the v4.1 milestone Aug 28, 2017

koppor mentioned this pull request Aug 31, 2017

[WIP] Create PDF file Exporter #2706

Closed

6 tasks

LinusDietz added the status: devcall label Aug 31, 2017

koppor added on-hold and removed status: devcall status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 31, 2017

Siedlerchr added 4 commits October 7, 2017 16:05

First approach for implementing PDF Exporter

9d38d12

Remove code that produces NPE

Add to File menu and context menu

50e9716

Fix localization to copy files

Remove separator from tools menu

26a7e1f

Siedlerchr force-pushed the exportPdf branch from d4858c8 to 26a7e1f Compare October 7, 2017 14:06

fix fileUtil and localzation from rebase

ec7c44b

Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed on-hold labels Oct 7, 2017

lenhard reviewed Oct 7, 2017

View reviewed changes

Remove empty line

68e1562

Avoid code duplicaiton

tobiasdiez requested changes Oct 8, 2017

View reviewed changes

Siedlerchr added 5 commits October 8, 2017 11:35

Move service to new class

d1b53eb

Fix translation

7021371

Add idea for list view dialog

6b02e9c

Merge remote-tracking branch 'upstream/master' into exportPdf

edec73d

* upstream/master: Set WM_CLASS to org-jabref-JabRefMain (#3273) Terminate JavaFX thread when running with "nogui" option (#3274)

fix controller

713ecec

tobiasdiez reviewed Oct 21, 2017

View reviewed changes

Siedlerchr added 4 commits October 21, 2017 12:17

add button data

4c01181

Add new wrapper class for injection

cc30b98

Add todos hinting at empty

a67cd4e

Siedlerchr commented Oct 21, 2017

View reviewed changes

Siedlerchr added 7 commits October 21, 2017 16:22

Fix injection of data

5d96526

Switch gui to border pane add tooltip to factory

rebame and fix checkstyle

8011e6e

Make variables private

bb1a304

Merge remote-tracking branch 'upstream/master' into exportPdf

5e1117a

* upstream/master: Initializing EntryEditor Tabs on focus (#3331) Fix #3292: annotations are now automatically refreshed (#3325) # Conflicts: # CHANGELOG.md

fix merge error in lang files

fc7a2a1

Fix some coday warnings

ba35360

LinusDietz reviewed Oct 25, 2017

View reviewed changes

Rename l10n

88faa7c

Add timestamp Improve GUI

Siedlerchr added 3 commits October 31, 2017 11:21

fix showing of progress bar

8f14cc7

Merge branch 'exportPdf' of https://github.com/JabRef/jabref into exp…

07ee6ea

…ortPdf * 'exportPdf' of https://github.com/JabRef/jabref: Rename l10n Add timestamp Improve GUI

Siedlerchr mentioned this pull request Oct 31, 2017

Call for function: export the pdfs of all the selected entrys to a new folder #2539

Closed

lenhard approved these changes Nov 1, 2017

View reviewed changes

lenhard merged commit d5f0b69 into master Nov 1, 2017

lenhard deleted the exportPdf branch November 1, 2017 15:28

Siedlerchr mentioned this pull request Oct 17, 2021

Saving a collection of entries/linked pdfs #8156

Closed


		public class ExportLinkedFilesAction extends AbstractAction {

		private final BiFunction<Path, Path, Path> resolvePathFilename = (p, f) -> {


		long totalFilesCount = entries.stream().flatMap(entry -> entry.getFiles().stream()).count();

		Service<Void> service = new Service<Void>() {

Uh oh!

Conversation

Siedlerchr commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Aug 31, 2017

Uh oh!

koppor commented Aug 31, 2017

Uh oh!

koppor commented Aug 31, 2017 • edited by Siedlerchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinusDietz commented Aug 31, 2017

Uh oh!

koppor commented Aug 31, 2017

Uh oh!

Siedlerchr commented Oct 7, 2017

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Aug 22, 2017 •

edited

Loading

koppor commented Aug 31, 2017 •

edited by Siedlerchr

Loading

LinusDietz left a comment •

edited

Loading