Follow-up #12035 and #12068: Show an informative progress indicator when dragging and dropping PDF files to JabRef by BaronMac08 · Pull Request #12072 · JabRef/jabref

BaronMac08 · 2024-10-24T09:30:14Z

Closes #8738
A follow-up to #12035 and #12068
Builds upon: the branch fix-melting-630 merged at #12001

Modifications:

Dragging and dropping several PDF files into JabRef will be one large import rather than one small import for every file

Additions:

Added informative messages about the ongoing progress of the big import by building upon the BackgroundTask progress indicator
Added shortenFileName(fileName, maxLength) in StringUtil to shorten and prominently display the files being imported
Added one new unit test for validating StringUtil.shortenFileName()

UI Screenshots after modifications and additions:

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

BaronMac08 · 2024-10-24T09:36:52Z

Completed manually adding changes from https://github.com/JabRef/jabref/pull/12035/files. Awaiting peer-review from @mag-sun before marking pull request as ready for review.

…added unit tests for maxLength null cases

BaronMac08 · 2024-10-25T10:24:22Z

Peer review from @mag-sun completed, the PR is now ready for review.

BaronMac08 · 2024-10-25T10:36:53Z

Hi @koppor, I have opened this PR as you requested in #12068. For the two failing unit tests:

StringUtil should have less than or equal to 774 lines of code
doNotUseLogicInModel was triggered due to the usage of FileUtil within StringUtil

Please let me know if there are any changes you would like me to make, thank you in advance for the feedback.

koppor · 2024-10-25T13:24:21Z

@BaronMac08 #12068 was closed. Maybe you mean #12072 -- please let pull requests open after changing something. Otherwise, it is really hard to follow.

koppor · 2024-10-25T13:26:02Z

2. doNotUseLogicInModel was triggered due to the usage of FileUtil within StringUtil

I think, not. import org.jabref.logic.util.io.FileUtil; is in logic, too. -- Please recheck.

koppor · 2024-10-25T13:27:11Z

StringUtil should have less than or equal to 774 lines of code

Move the code to FileUtil. I think, the cohesion is better there.

BaronMac08 · 2024-10-25T21:34:35Z

I have moved the shortenFileName() method to FileUtil, as @koppor suggested. All checks have passed.

koppor

Small comments on the functionality of the ellipses. Maybe, you can fix it?

koppor · 2024-10-26T20:56:16Z

+            "...                            , longfilename.extremelylongextension ,  5",
+            "...                            , longfilename.extremelylongextension , 10",
+            "...                            , longfilename.extremelylongextension , 20",


This is not good behavior.

Expected:

Suggested change

"... , longfilename.extremelylongextension , 5",

"... , longfilename.extremelylongextension , 10",

"... , longfilename.extremelylongextension , 20",

"lo.. , longfilename.extremelylongextension , 5",

"longfil... , longfilename.extremelylongextension , 10",

"longfilename.extr.... , longfilename.extremelylongextension , 20",

Reason: There are plenty !! of characters available at 10 and 20. They should be used. Not just 3

koppor · 2024-10-26T20:56:28Z

+            "''                             , thisisatestfile.pdf                 , -5",
+            "''                             , thisisatestfile.pdf                 ,  0",
+            "...                            , thisisatestfile.pdf                 ,  3",
+            "...                            , thisisatestfile.pdf                 ,  5",


Please! Use all 5 characters: th....

Definitely, I will modify the functionality to make use of the available characters.

koppor · 2024-10-26T21:03:17Z

Additional comment:

The number of files is displayed multiple times:

Please change progress information before the pipe sign to Importing files into 12072.bib |

And if you are on it, can you also change it for the indexing string?

koppor · 2024-10-26T21:08:30Z

Drop of files does not work. ALL files are attached to ALL entries. Even on pressing Cancel.

Please try out your things. Having 500 PDFs available is not too hard. I used all from my Downloads folder to test. - If not, you can try out the PDFs available at https://github.com/koppor/tugboat-mirror/tree/main/tugboat_issues.

…dated progress messages for Importing, Indexing files.

BaronMac08 · 2024-10-26T23:37:50Z

Drop of files does not work. ALL files are attached to ALL entries. Even on pressing Cancel.

Please try out your things. Having 500 PDFs available is not too hard. I used all from my Downloads folder to test. - If not, you can try out the PDFs available at https://github.com/koppor/tugboat-mirror/tree/main/tugboat_issues.

Thank you for pointing out the issue. I believe the issue is caused by the grouping of all files' entries and then sending it to the indexManager as one big EntriesAddedEvent. Therefore, I perform the exact same test of dragging and dropping the PDF files provided at https://github.com/koppor/tugboat-mirror/tree/main/tugboat_issues into JabRef/main and branch fix-8738 with the following results:

On JabRef/main:
On fix-8738:

The difference between main and fix-8738 for the entry "... To Don Knuth..." in the linked files section:

On main: tb139complete.pdf and tb138complete.pdf are at the beginning of the list
On fix-8738: tb138complete.pdf and tb139completed.pdf are at the end of the list

Additionally, on both main and fix-8738, the file "The Communications of the TEX Users Group.pdf" has the same linked file.

I am wondering what would be the correct behaviour for this dataset of PDF files so that the issue on branch fix-8738 could be identified and resolved. Thank you in advance for the clarification @koppor.

koppor · 2024-10-26T23:45:11Z

I am wondering what would be the correct behaviour for this dataset of PDF files so that the issue on branch fix-8738 could be identified and resolved.

Thank you for investigating. I needed to raise a new issue at #12098.

koppor · 2024-10-26T23:45:52Z

Thank you for the quick follow-up in the filename methods. This is good to go now!

BaronMac08 · 2024-10-26T23:48:41Z

Thank you for your guidance and advice!

BaronMac09 added 2 commits October 24, 2024 20:25

Updated changes following up for pull requests #12035 and #12068

07d0785

Updated CHANGELOG.md to display correct pull request

fb75d00

mag-sun reviewed Oct 25, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread src/main/java/org/jabref/model/strings/StringUtil.java Outdated

Comment thread src/test/java/org/jabref/model/strings/StringUtilTest.java Outdated

BaronMac09 added 2 commits October 25, 2024 21:17

Updated StringUtil.shortenFileName to handle maxLength null case and …

84b40b8

…added unit tests for maxLength null cases

Updated CHANGELOG.md to follow format and be more concise

0e0a1a1

BaronMac08 marked this pull request as ready for review October 25, 2024 10:23

Moved shortenFileName() to FileUtil

0f26919

koppor requested changes Oct 26, 2024

View reviewed changes

koppor added the status: changes-required Pull requests that are not yet complete label Oct 26, 2024

Refactored FileUtil.shortenFileName to better use of maxLength and Up…

50fc657

…dated progress messages for Importing, Indexing files.

koppor mentioned this pull request Oct 26, 2024

Dropping multiple PDFs leads to a wrong file field #12098

Closed

2 tasks

koppor approved these changes Oct 26, 2024

View reviewed changes

koppor added this pull request to the merge queue Oct 26, 2024

Merged via the queue into JabRef:main with commit 61d342a Oct 27, 2024

This was referenced Dec 23, 2024

Draft blog post for 6.0 alpha JabRef/blog.jabref.org#107

Merged

Merge names koppor/github-contributors-list#41

Open

Uh oh!

Conversation

BaronMac08 commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mandatory checks

Uh oh!

BaronMac08 commented Oct 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BaronMac08 commented Oct 25, 2024

Uh oh!

BaronMac08 commented Oct 25, 2024

Uh oh!

koppor commented Oct 25, 2024

Uh oh!

koppor commented Oct 25, 2024

Uh oh!

koppor commented Oct 25, 2024

Uh oh!

BaronMac08 commented Oct 25, 2024

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

koppor Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

koppor Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

BaronMac08 Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

koppor commented Oct 26, 2024

Uh oh!

koppor commented Oct 26, 2024

Uh oh!

BaronMac08 commented Oct 26, 2024

Uh oh!

koppor commented Oct 26, 2024

Uh oh!

koppor commented Oct 26, 2024

Uh oh!

BaronMac08 commented Oct 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BaronMac08 commented Oct 24, 2024 •

edited

Loading