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.

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 for delete entries should ask user #10591

Merged
merged 59 commits into from
Feb 26, 2024

Conversation

shawn-jj
Copy link
Contributor

@shawn-jj shawn-jj commented Oct 27, 2023

Fixes #10509

Mandatory checks

  • 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.

Implemented the following features

  • Create issue #471 for user documentation
  1. When user delete entries, files which linked to selected entries are also delete

  2. When user cut entries, keep files unchanged

  3. Solved the access error caused by repeated deletion of files when one file is attached to multiple entries

  4. A pop-up dialog box to confirm whether the user wants to delete attached files from selected entry

  5. Keep track of user preference: if the user prefers not show confirmation dialog when deleting entries, then delete the files without displaying the dialog box

    image
  6. Add a preference option in File>Preference>Linked Files>Attached files so that users can manage preference about external files deletion

    image

…entries when user select deletion, and keeping files unchanged when user select cut
… box to confirm whether the user wants to delete attached files from selected entry. 2.Keep track of user preference: if the user prefers always delete attached files, delete the files without displaying the dialog box. 3. Add preference options in File>Preference>Linked Files>Attached files so that users can manage preferences
@Siedlerchr Siedlerchr changed the title Fix for issue 10509 Fix for delete entries should ask user Oct 27, 2023
@shawn-jj
Copy link
Contributor Author

I am trying to fix the pipeline, but OpenRewrite test is always failing. The log is provided below, and it appears to be connected to the recent code modifications I made in LibraryTab.java. However, I'm having difficulty finding the exact source of the problem.

Greatly appreciate it if someone could give me some hints!

> Task :rewriteDryRun
Validating active recipes
Scanning sources in project :
Using active styles [org.openrewrite.java.Checkstyle]
Failed to resolve dependencies from ::implementation. Some type information may be incomplete
Failed to resolve dependencies from ::testImplementation. Some type information may be incomplete
All sources parsed, running active recipes: org.jabref.config.rewrite.cleanup
These recipes would make changes to src/main/java/org/jabref/gui/LibraryTab.java:
    org.jabref.config.rewrite.cleanup
        org.openrewrite.staticanalysis.IsEmptyCallOnCollections

Report available:
FAILURE: Build failed with an exception.
    /home/runner/work/jabref/jabref/build/reports/rewrite/rewrite.patch

Run 'gradle rewriteRun' to apply the recipes.
* What went wrong:

Execution failed for task ':rewriteDryRun'.
> Task :rewriteDryRun FAILED
> java.lang.RuntimeException: Applying recipes would make changes. See logs for more details.

@koppor
Copy link
Member

koppor commented Oct 28, 2023

I am trying to fix the pipeline, but OpenRewrite test is always failing. The log is provided below, and it appears to be connected to the recent code modifications I made in LibraryTab.java. However, I'm having difficulty finding the exact source of the problem.

Please execute the rewriteRun task. This does the change in the file.

koppor
koppor previously requested changes Oct 28, 2023
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
… new features: 1. When deleting attached files, the name of files to be deleted will be displayed. 2. Solved the access error caused by repeated deletion of files when one file is attached to multiple entries.
@shawn-jj shawn-jj marked this pull request as ready for review November 2, 2023 05:15
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Small comments - did not have time to completely check

src/main/java/org/jabref/gui/LibraryTab.java Outdated Show resolved Hide resolved
shawn-jj and others added 4 commits November 3, 2023 20:49
* upstream/main:
  fix tests
  Bump org.jsoup:jsoup from 1.16.1 to 1.16.2 (JabRef#10625)
  Bump org.junit.jupiter:junit-jupiter from 5.10.0 to 5.10.1
  Bump org.fxmisc.flowless:flowless from 0.7.1 to 0.7.2
  Bump com.dlsc.gemsfx:gemsfx from 1.84.0 to 1.90.0
  Bump com.tngtech.archunit:archunit-junit5-api from 1.1.0 to 1.2.0
  Change linter (JabRef#10614)
  change codecov (JabRef#10616)
  Add entry based on ISSN number JabRef#10124 (JabRef#10178)
  Enable journal information fetcher directly in popup (JabRef#10598)
Siedlerchr
Siedlerchr previously approved these changes Nov 6, 2023
@Siedlerchr Siedlerchr enabled auto-merge November 6, 2023 20:04
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 25, 2024
@ThiloteE
Copy link
Member

ThiloteE commented Jan 25, 2024

Test with default preferences from older version / main.

  1. Select entry

  2. Delete entry via key "Delete" or right-click context menu and choose "Delete entry"

  3. Triggers:
    image

    • Choose "Keep entry".
      Result: Entry is kept. Attached file is not deleted. 👍
    • Choose "Delete entry"
      Result: Entry is deleted. Next popup dialogue triggers. 👍
      image
    • Choose "Cancel"
      Result: Entry is deleted. Attached file is not deleted. 👍
    • Press "CTRL + Z" for Undo after step 6.
      Result: Entry will re-emerge. Attached file will continue to be attached to the entry. Requirement: Only works, if pressing "ctrl + z" TWICE. Could be better, by only having to press ctrl + z once, but good enough for release, so 👎 👍
    • Choose "Remove from entry"
      Result: Entry is deleted. Attached file is not deleted. 👍
    • Press "CTRL + Z" for Undo after step 8.
      Result: Similar results as in Step 7. 👎 👍
    • Choose "Delete from Disc"
      Result: Entry is deleted. Attached file is deleted from disk. Actually, file is in trash, even though users default preference is
      image
      Expected behaviour with these settings: File should not be in trash, but permanently deleted. So, kinda 👎 ❌ 👍
    • Press "CTRL+Z" for Undo twice after Step 10.
      Result: Entry re-emerges. 👍 Attached file is still deleted from disk. (is in trash) 👎 ❌
      Expected behaviour: File should not be in trash, but back in the folder and attached to the file.
    • Enable the following preference: Move deleted files to trash (Instead of deleting them), then choose "Delete from disk"
      Result: Entry is deleted. Attached file is deleted from disk. Actually, file is in trash. 👍
    • Select multiple entries and choose "delete from disk"
      Result: Entries are deleted. Attached file is deleted from disk. Actually, file is in trash. 👍 Undo works too.

Final observations / recommendation:

  • Enabling or disabling the preference does not seem to change anything, so that should either be fixed or the preference be removed.
  • The default preference should be to move attached files upon deletion into the trash, to prevent dataloss. Therefore, Move deleted files to trash (Instead of deleting them) should be enabled by default, not disabled.
  • You may have to press "undo" multiple times in certain circumstances.

@koppor koppor marked this pull request as draft January 27, 2024 22:16
@koppor
Copy link
Member

koppor commented Jan 27, 2024

@ThiloteE Thank you for testing. I had the fear that "Trash" is not activated upon upgrade to this development version. We need to check.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 5, 2024
@koppor
Copy link
Member

koppor commented Feb 18, 2024

New dialog (question more generic, because of Trash)

grafik

# Conflicts:
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
@koppor koppor marked this pull request as ready for review February 18, 2024 22:24
@koppor
Copy link
Member

koppor commented Feb 19, 2024

@ThiloteE I missed to bind the preferences UI correctly with the internal preferences. Thus, the UI setting had no effect. I fixed it. -- It should be good to go. @Siedlerchr will also test it on MacOS and then we will merge this in. -- Regarding the undo, I created an internal follow-up issue at https://github.com/JabRef/jabref-issue-melting-pot/issues/358

koppor and others added 2 commits February 19, 2024 14:35
* upstream/main: (22 commits)
  Bump com.github.andygoossens.modernizer from 1.9.0 to 1.9.2 (JabRef#10922)
  Bump com.dlsc.gemsfx:gemsfx from 1.97.0 to 2.0.3 (JabRef#10923)
  Bump org.apache.logging.log4j:log4j-to-slf4j from 2.22.1 to 2.23.0 (JabRef#10921)
  Added missing changelog entry for JabRef#10912
  Change the popup to enter types to the combo box for custom entry types (JabRef#10912)
  Add JDK EA build (JabRef#10904)
  Fix Broken Links (JabRef#10899)
  Add HTML2MD conversion to abstract and comment fields (JabRef#10896)
  docs: Fixed URLs and corrected typos (JabRef#10900)
  Add refresh button for LaTeX citations. (JabRef#10901)
  Farewell btut 👋 (JabRef#10905)
  Fix: About OOError, Alternatives section not visible (JabRef#10902)
  Update code-quality.md
  Fix documentation issues: Sourcegraph URL and method name (JabRef#10898)
  Removed mainapplication layer (JabRef#10895)
  Update check-links.yml (JabRef#10897)
  [WIP] Adds the ability to specify [camelN] as a title-related field marker for citation key generation (JabRef#10772)
  Bump com.dlsc.gemsfx:gemsfx from 1.92.0 to 1.97.0 (JabRef#10894)
  Bump org.openrewrite.recipe:rewrite-recipe-bom from 2.6.3 to 2.6.4 (JabRef#10892)
  Bump org.junit.platform:junit-platform-launcher from 1.10.1 to 1.10.2 (JabRef#10893)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/desktop/os/NativeDesktop.java
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 26, 2024
@Siedlerchr
Copy link
Member

Works on mac

@Siedlerchr Siedlerchr enabled auto-merge February 26, 2024 18:58
@Siedlerchr Siedlerchr disabled auto-merge February 26, 2024 18:59
@Siedlerchr Siedlerchr enabled auto-merge February 26, 2024 19:00
@Siedlerchr Siedlerchr dismissed koppor’s stale review February 26, 2024 19:00

To get merge ready

@Siedlerchr Siedlerchr added this pull request to the merge queue Feb 26, 2024
Merged via the queue into JabRef:main with commit bec55f8 Feb 26, 2024
21 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 27, 2024
* issue #10661 - feat: added option to export in cff

* issue #10661 - feat: updated CHANGELOG.md

* issue #10661 - feat: added date field to cff export

* issue #10661 - fix: fixed checkstyle errors

* Bump org.apache.lucene:lucene-queries from 9.9.1 to 9.10.0 (#10920)

* Bump org.apache.lucene:lucene-queries from 9.9.1 to 9.10.0

Bumps org.apache.lucene:lucene-queries from 9.9.1 to 9.10.0.

---
updated-dependencies:
- dependency-name: org.apache.lucene:lucene-queries
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* introduce var for lucene

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>

* Fix for delete entries should ask user (#10591)

* Implemented the feature that deleting files which linked to selected entries when user select deletion, and keeping files unchanged when user select cut

* The following features are implemented: 1.Initializes a pop-up dialog box to confirm whether the user wants to delete attached files from selected entry. 2.Keep track of user preference: if the user prefers always delete attached files, delete the files without displaying the dialog box. 3. Add preference options in File>Preference>Linked Files>Attached files so that users can manage preferences

* update CHANGELOG.md

* Add language keys to english language file

* restore files in src/main/resources/csl-locales and src/main/resources/csl-styles

* Removed unnecessary comments and finxed some requested changes. Added new features: 1. When deleting attached files, the name of files to be deleted will be displayed. 2. Solved the access error caused by repeated deletion of files when one file is attached to multiple entries.

* Add language keys to english language file

* Modify language keys to english language file

* made deleteFileFromDisk method static

* update comment of method deleteFileFromDisk

* fixed coding styles

* restored unexpected code changes

* fix logic

* try null

* todo

* Unify dialogs that confirmation deleting files

* Get around LinkedFile in LIbraryTab, Encapsulate LinkedFile into LinkedFileViewModel

* fix style

* restore files

* Unified the different dialogs when deleting entries, removerd unnecessary dialogs

* fix csl-styles

* try to fix csl-styles

* try to fix csl-styles again

* try to fix csl-styles again 2

* try to fix csl-styles again 3

* Update prompts in en.properties

* New features

- Add to Trash
- Group file-related language strings together

* Fix architecture tests

* Introduce list of files to delete

* Streamline 1 vs. many files

* Fix openRewrite

* Discard changes to src/test/resources/org/jabref/logic/search/test-library-with-attached-files.bib

* Adapt true/false logic according to expectations

* Add "Trash" to CHANGELOG.md

* Fix localization

* Fix JabRef_en.properties

* Add some debug statements

* Fix preferences

* Separate log entries by empty line

* More refined dialog

---------

Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

* Fix variable name

* Fix duplicate check/merge entries dialog not triggered on import from… (#10914)

* Fix duplicate check/merge entries dialog not triggered on import from browser


Refs #5858

* changelog

* remove double duplicate check

* remove l10n

* add icon, downloading is also handled in import entries

* changelog

* fix l10n

* Update JabRef_en.properties

* Update ImportEntriesDialog.java

---------

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

* issue #10661 - fix: fixed PR review comments

Made a class comment in CffDate.java
Replaced System.lineseparator() with OS.NEWLINE in CffDate.java
Added a final newline in cff.layout file

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
Co-authored-by: shawn-jj <134609685+shawn-jj@users.noreply.github.com>
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
@koppor koppor mentioned this pull request Mar 4, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external files status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete entry should ask to delete linked files
5 participants