-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Fix several File Cleanup + Rename issues #2415
Conversation
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
* upstream/master: (67 commits) Medline fix test (#2463) Fix conversion of tilde n (#2459) ctrl+f selects current query while the searchbar is focusd (#2457) incorrect log name of JabRefExecutorService (#2452) unregister DateChangeListener in manual update method (#2450) Escaping of escape symbols in the MetaData (#2445) Fix typo Update gradle from 3.2.1 to 3.3 Use instanceof Remove unused import Avoid ClassCastException in AutoCompleteListener Fix typo in CHANGELOG.md Add support for pages in the format 2:1-2:33 (#2440) L10N-ru update (#2441) Change https to http Update DBLP API endpoint Revert "Chistmas edition colors" Show development information Release v3.8.1 Result of generate-authors.sh ... # Conflicts: # CHANGELOG.md # src/main/java/net/sf/jabref/gui/date/DatePickerButton.java
GUI: CleanupPreset Panel + Metadata.getDefaultFileDirectory seems not to check bibtex location somehow |
… other file directories Enable move files cleanup checkbox then, too Mock prefs in tests
* upstream/master: Save deletion of current searchquery (#2469) Update dev dependencies (mockito, wiremock, assertj) Update BouncyCastle (1.55->1.56), ANTRL4 (4.5.3->4.6), jsoup (1.10.1->1.10.2) Group all checker which only check the value of one field (#2437) Follow up #2428 (#2438) Fix conversion of "'n" and "\'{n}" from LaTeX to Unicode (#2464) Fix failing tests
* upstream/master: Use Arrays.stream
* upstream/master: Fix medline tests...again (#2492) Make sure that unregistered event sources do not stop JabRef from shu… (#2487) Fix #2481: ClassCastException because of wrong cast (#2490) Catch NumberFormatException if context can't be parsed in groups (#2488) Improve CHANGELOG formatting Update guava from 20.0 to 21.0 and mockito-core from 2.5.5 to 2.6.2 Fix aux duplicates (#2480) add update from DOI to the entryeditor sidebar (#2476) Remove obsolete import Add CHANGELOG entry (and one more link) Resolves #2309 JabRef freezes when importing unlinked PDF files into Database Update CHANGELOG.md Fixed bug when assigning refs to groups. emove html code from ACM fetcher before calling parser to prevent junk in bib file (#2473)
* upstream/master: (82 commits) Add access Rule for javafx to avoid Acces restricted in eclipse Fix apostrophe conversion (#2507) Fix Alain's name rename LocalizationBundle in Test folder (#2504) Fix eclipse bug again Update CHANGELOG.md Adapt IntelliJ Code Style.xml to checkstyle.xml Fix @param declaration and remove author to be consistent with other code Resolve conflicts using the versions javafx branch, not the master branch (and use alt+f8 instead of f8) Show development information Release v3.8.2 Always override JabRef.vmoptions in the installation folder Update install4j from 6.1.3 to 6.1.4 Update AUTHORS wget for new JREs Try vmOptionsFile Update JRE from 112 to 121 Change key binding for cleanup from F8 to ALT+F8 (#2496) Use https URL of www.jabref.org Save test reports as build artifacts on circle ci ...
TODO: Find a way to cleanup only one file
|
|
Add fileDirPattern to Rename File
* upstream/master: Fix error when path is no valid directory (#2527) French localization: translation of a string French menu: localization Highlight groups that match any/all of the entries selected in the main table. (#2515) Fix % sign cleanup (#2521) Revert "Fix repeated escaping of % sign" (#2520) Fix repeated escaping of % sign (#2519) fix for #2482 deadlock on PDF import (#2517)
Rename only performs a rename no longer a move
|
* upstream/master: Switch to Latex2unicode (#2532)
FileDirPattern is only used in MoveFiles Better distinction between rename and move
Rework tests, delete tests with filedirpattern
* upstream/master: (183 commits) Reorder pferences tab to be more intuitive (#2560) Reoder translations to be more meaningful move jmh benchmark fix prefs import test Add prefs migration add missing files replace text occurrences fix logging fix imports fix intellij code style fix jaxb based importers fix search generation move and fix tests fix compilation errors update checkstyle for correct positioning of org.jabref packages fix wrong positioning of some gui classes fix generation of sources second part of gui first part of gui second part of logic move ... # Conflicts: # src/main/java/org/jabref/gui/externalfiles/MoveFileAction.java # src/main/java/org/jabref/gui/fieldeditors/FileListEditor.java # src/main/java/org/jabref/gui/filelist/FileListEntry.java # src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java # src/test/java/org/jabref/logic/cleanup/CleanupWorkerTest.java # src/test/java/org/jabref/logic/cleanup/MoveFilesCleanupTest.java
* upstream/master: Fix colour of tooltip Groups: click on arrow should toggle expansion status Fix import of MS Office xml in case of invalid month (#2538) Change MainClass from net.sf to org.jabref.JabRefMain
* upstream/master: Revert "Update gradle from 3.3 to 3.4" Localization tests (#2582) Update IEEEJournalList (#2579) Keyword - Special field synchronization (#2583) Update gradle from 3.3 to 3.4 Check similarity of entry when using DOI retrieval with ArXiV (#2575) Add logic for new Sciencedirect pages (#2576) [WIP] Refactored around the FileAnnotationCache. (#2557)
Add changelog enty and missing dots in previous entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a brief review, maybe I find more time later for more detailed feedback.
In general the code looks really good! Thanks for you effort...everything file related is kind of a hell in JabRef! It is now a lot better.
@@ -34,6 +34,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
- We integrated support for the [paper recommender system Mr.DLib](http://help.jabref.org/en/EntryEditor#related-articles-tab) in a new tab in the entry editor. | |||
- We renamed "database" to "library" to have a real distinction to SQL and NoSQL databases. [#2095](https://github.com/JabRef/jabref/issues/2095) | |||
- Removed the apache.commons.collections library | |||
- The `Move linked files to default file directory`-Cleanup operation respects the `File directory pattern` setting | |||
- We separated the `Move file` and `Rename Pdfs` logic and context menu entries in the `General`-Tab for the Field `file` to improve the semantics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup, tab and field, all with a small first letter
private final JabRefFrame frame; | ||
private final EntryEditor eEditor; | ||
private final FileListEditor editor; | ||
private final CleanupPreferences prefs = Globals.prefs.getCleanupPreferences(new JournalAbbreviationLoader()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the actions are initialized once at the start of JabRef. Thus changes in the cleanup preferences are not recognized with this code. (there should also be a journal abbreviation loader in globals)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now pass the prefs directly
} | ||
|
||
@Override | ||
public List<FieldChange> cleanup(BibEntry entry) { | ||
if(!databaseContext.getMetaData().getDefaultFileDirectory().isPresent()) { | ||
Optional<Path> firstExistingFileDir = databaseContext.getFirstExistingFileDir(fileDirectoryPreferences); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that so much logic is hidden in a cleanup process. In my opinion it would be better to extract as much logic as possible to separate classes and reuse them here and also in the other places (instead of reusing the cleanup).
In more detail I propose to:
- Create a class that wraps around
List<ParsedFileField>
and that takes care of moving all linked files (and providing nice helper methods likereplace(one file, by another one)
- Almost everything in the for loop below can be extracted to the ParsedFileField class (and be split in smaller helper methods like ParsedFileField.exists())
- The code is a bit complicated since you constantly have to pass the fileDirectoryPreferences around (not your fault!). What you think about passing these information once into
TypedBibEntry.getFiles
as arguments, which are then stored in the ParsedFileField class? Thus the ParsedFileField class actually knowns if the file exists in one of the directories, what its preferred name is, can rename and move itself...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea. However, as my time is currently limited due to exams I would rather vote to merge it in as it is, as it fixes several important bugs and I would create a followup PR for making the code strucutred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its probably fine... maybe somebody else should also have a look at the code before merge.
* upstream/master: (21 commits) Implement #1904: filter groups (#2588) Braces checking followup (#2598) Improve braces checking (#2593) Fix pdf file import when only one file selected and no entry created (#2592) Fix test Imports Localization: MainFile: French: update (#2591) isPdf helper method Add asString with UTF-8 as standard encoding Fix names of groups in "add/move to group" dialog Make automatic groups a first-class citizien (#2563) Rename downloadToString method to asString Refactoring method names Inline Mime type detection Fix imports Extract SSL bypassing Bypass SSL functionality Unused methods and refactoring Always use browser user agent Monitored URLDownload is currently not use anywhere ... # Conflicts: # CHANGELOG.md
I had a look at the code, which in general is good, and fixed a few minor issues directly (missing getters/setters that were missing well before this PR, but this is a good time for a fix). I also played around a little with the UI and tried to find errors, but was unsuccessful. In the spirit of progress and as suggested by @Siedlerchr #2415 (comment) I am merging now directly. More far fetching suggestions by @tobiasdiez should be addressed in a follow-up PR, as discussed above. @Siedlerchr Thanks again for this heap of work! |
* upstream/master: (91 commits) fixed #2613 (#2623) Add MathSciNet as ID-based fetcher (#2621) Add icon + color and description to groups (#2612) Fixed wrong logger import (#2618) Cleanup window has a scrollbar now. (#2614) Added the locale to a newly created class Move ExportComparator and CustomExportList to the correct package (only used in preferences) Fixes displaying of Mr DLib recommendations (#2616) Fix title-related key patterns in BibtexKeyPatternUtil (#2610) Remove OrdinalsToSuperscriptFormatter from recommended biblatex save actions (#2601) Update pgjdbc to new major version Update Dependenices String Similary log4j wiremock mockito Fix exception when parsing groups which contain a top level group (#2611) Add "Remove group and subgroups" option (#2587) Fix #1104: group selection is preserved under tab change Fix several File Cleanup + Rename issues (#2415) Fixed a small error in the code comments Implement #1904: filter groups (#2588) Braces checking followup (#2598) Improve braces checking (#2593) ...
Separated Move + Rename Cleanup Logic
Move + Rename files in General Tab -> File now use internally the cleanup operations logic
Separated GUI context menu operations for move + cleanup
Rename files no longer moves files, it only renames them.
Files are not overwritten if they exist or have the same name.
Fix for #2385 and followup from #1899
Includes Fix for #2454 and #2526
gradle localizationUpdate
?