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

Deleting files move them to trash #963

Merged
merged 4 commits into from
Aug 11, 2023
Merged

Deleting files move them to trash #963

merged 4 commits into from
Aug 11, 2023

Conversation

juuz0
Copy link
Collaborator

@juuz0 juuz0 commented Jul 16, 2023 •
edited
Loading

Fixes #479

@juuz0 juuz0 requested a review from kelson42 July 16, 2023 21:21
@kelson42
Copy link
Collaborator

@juuz0 Superseeds #531?

@juuz0 juuz0 changed the base branch from main to qtlibrary July 27, 2023 13:14
@kelson42
Copy link
Collaborator

@juuz0 Superseeds #531?

@juuz0 ???

mgautierfr
mgautierfr requested changes Jul 28, 2023
View reviewed changes
resources/i18n/en.json Outdated Show resolved Hide resolved
src/kiwixapp.cpp Outdated Show resolved Hide resolved
src/settingsmanager.cpp Outdated Show resolved Hide resolved
src/settingsmanager.h Outdated Show resolved Hide resolved
src/settingsmanager.h Show resolved Hide resolved
src/settingsview.cpp Outdated Show resolved Hide resolved
@juuz0 juuz0 force-pushed the qtlibrary branch 4 times, most recently from 2c2046a to 566902c Compare July 30, 2023 17:08
@juuz0
Copy link
Collaborator Author

juuz0 commented Jul 31, 2023

Superseeds #531?

@kelson42 Yes.

@juuz0 juuz0 requested a review from mgautierfr July 31, 2023 10:30
mgautierfr
mgautierfr requested changes Jul 31, 2023
View reviewed changes
src/library.cpp Outdated Show resolved Hide resolved
src/kiwixapp.cpp Outdated Show resolved Hide resolved
Base automatically changed from qtlibrary to main August 1, 2023 10:31
@juuz0 juuz0 requested a review from mgautierfr August 1, 2023 12:53
@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 1, 2023

I've a concern with this PR - this won't work on focal. moveToTrash was added in Qt 5.15, but focal is 5.12.
I see 2 solutions when QT version < 5.15:

  1. write custom platform specific code to move to trash. I am not exactly sure how this would work, will search and see.
  2. disable the option to move to trash when the version is lower than 5.15

@kelson42
Copy link
Collaborator

kelson42 commented Aug 1, 2023

I've a concern with this PR - this won't work on focal. moveToTrash was added in Qt 5.15, but focal is 5.12.

@juuz0 Thank you for raising this point. Here many remarks regarding this:

I believe your approach is very pragramatic and if the (2) option is not too complicated, please implement it. On our side we should really consider to move to Qt15.5 in our compilation toolchain.

@mgautierfr mgautierfr mentioned this pull request Aug 1, 2023
Closed
kelson42
kelson42 approved these changes Aug 1, 2023
View reviewed changes
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

From a functional perspective, tested on Ubuntu 22.04, it works perfectly to me.

@juuz0
Copy link
Collaborator Author

juuz0 commented Aug 3, 2023

I believe your approach is very pragramatic and if the (2) option is not too complicated, please implement it.

Done

mgautierfr
mgautierfr requested changes Aug 4, 2023
View reviewed changes
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

A renaming of the mutex to do.

I'm not sure about other renaming. I'm open to discussion here. (Or you can directly do the change if you agree with me)

src/library.h Outdated Show resolved Hide resolved
src/library.cpp Outdated Show resolved Hide resolved
src/library.h Outdated Show resolved Hide resolved
@juuz0 juuz0 requested a review from mgautierfr August 8, 2023 04:13
mgautierfr
mgautierfr requested changes Aug 10, 2023
View reviewed changes
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Few comments on the last commit.

src/contentmanager.cpp Outdated Show resolved Hide resolved
src/contentmanager.cpp Outdated Show resolved Hide resolved
@kelson42
Copy link
Collaborator

@juuz0 Please fix the last details so we can merge before the WE.

juuz0 added 4 commits August 11, 2023 22:04
@juuz0
New settings entry: Move files to trash
0f24892 
Added a new settings entry to toggle if deleted files should move to trash/recycle bin.
@juuz0
Allow user to choose between deleting permanently or moving to trash
ea51737 
Added functionality to delete files to trash or deleting them permanently.
@juuz0
Always monitor download directory
5ee18f1 
This change puts download directory in an always monitoring state.
For any files downloaded using kiwix downloader (and hence saved in download directory), this will automatically update the library if file is restored after deletion.
@juuz0
Remove moveToTrash feature for QT versions pre 5.15
e7dc33a 
The settings entry is hidden when the QT version is not supported
The file is always permanently deleted - dialog box indicates this.
@juuz0 juuz0 requested a review from mgautierfr August 11, 2023 16:35
@kelson42
Copy link
Collaborator

@juuz0 Thank you for the last cosmetic fix, I will merge.

@kelson42 kelson42 merged commit 1ad4a72 into main Aug 11, 2023
@kelson42 kelson42 deleted the moveToTrash branch August 11, 2023 18:08
@mgautierfr mgautierfr mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelson42 kelson42 kelson42 approved these changes

@mgautierfr mgautierfr Awaiting requested review from mgautierfr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deleting a ZIM file should move it to the trash can
3 participants
@juuz0 @kelson42 @mgautierfr