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

Add option to reset book and remove from recents screen (#851) #946

Merged
merged 14 commits into from
Aug 9, 2024

Conversation

Haaruun-I
Copy link
Contributor

@Haaruun-I Haaruun-I commented Jul 25, 2024

Adds a 'Reset Book' button to both the context menu in the preview card and the book details page.

For now all it does is remove the 'last_played' time to get it off the recents screen, but i'm not sure if it should also reset the playback progress.

Not sure about the wording of the options though, reset sounds like it should also clear your playback, but 'remove from recents' is too long.

Heres it in action, still cant get the icons to work (#852) current 'reset' icon is the back arrow also used for the seek bar

Screencast.from.2024-07-25.20-52-40.mp4

@geigi
Copy link
Owner

geigi commented Jul 26, 2024

Thanks for your contribution :) Removing books from the recent page again is a nice feature I think.

Some thoughts on the feature, haven't looked at the code yet:

  • I agree with the wording as it is unclear what reset means and what it does. I would opt for the longer option "remove from recents" for the context menu
  • A reset for the playback position (progress) is not really necessary in my opinion because you can just click on the first chapter and start again
  • I would prefer to keep this option only in the context menu of the library view. The current action (removing from recent page) is completely unrelated to the book detail page. I also feel like providing this action a button which is almost as prominent as the play button is confusing.

@Haaruun-I
Copy link
Contributor Author

Alright!

Copy link
Collaborator

@rdbende rdbende left a comment

Choose a reason for hiding this comment

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

I left a few comments, but it's mostly unneded code.
Thanks a lot for working on this, btw! I now have some hope that we can get some new contributors to the project 😊

cozy/ui/library_view.py Outdated Show resolved Hide resolved
cozy/ui/book_detail_view.py Outdated Show resolved Hide resolved
cozy/ui/widgets/book_card.py Outdated Show resolved Hide resolved
data/ui/book_card.blp Outdated Show resolved Hide resolved
Haaruun-I and others added 4 commits July 26, 2024 22:33
Co-authored-by: Benedek Dévényi <rdbende@proton.me>
Co-authored-by: Benedek Dévényi <rdbende@proton.me>
Co-authored-by: Benedek Dévényi <rdbende@proton.me>
@Haaruun-I
Copy link
Contributor Author

Haaruun-I commented Jul 26, 2024

Thank you for the quick review! I use cozy allot, and I had some free time, so I just thought I'd help out a bit. :D

Copy link
Collaborator

@rdbende rdbende left a comment

Choose a reason for hiding this comment

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

Hi @Haaruun-I! I fixed the issue you had to work around in your "Temporary UI Bugfix". Sorry about that, it was my bad. Somehow I didn't commit that hunk in one of my PRs.

@rdbende rdbende linked an issue Aug 9, 2024 that may be closed by this pull request
@rdbende rdbende merged commit 98e034d into geigi:master Aug 9, 2024
4 checks passed
@Haaruun-I
Copy link
Contributor Author

Thanks @rdbende! Dont know how I missed the BookCardPlayButton was right there 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Items from Recents
3 participants