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

feat: book reader swipe pagination #2754

Draft
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

ShivamAmin
Copy link
Contributor

@ShivamAmin ShivamAmin commented Feb 29, 2024

Added

  • Added: Setting to toggle swipe to paginate setting in reader settings
  • Added: Disable text highlighting when menu is hidden and swipe to paginate is enabled
  • Added: Swipe to change page functionality using swipe distance (30% screen threshold) (Idea: [Kavita] Swipe pagination on epub reader #2666)
  • Added: Migrated DB to add new user preference setting: BookReaderSwipeToPaginate
  • Added: User Configurable Swipe To Paginate Threshold User Preferences settings

Changed

Fixed

  • Fixed: Fixed a bug where the user is able to soft lock the book reader by enabling immersive mode but disabling click to paginate
  • Fixed: Disable click to paginate boxes when action bar is visible (fixes Tap pagination causes issues with text selection (epub) (android) #2694)
  • Fixed: User preferences page so that Tap and Swipe pagination are mutually exclusive

TODO

  • Nice to have: Swipe animations (May be more effort than worth?)
  • Suggestions welcome

@ShivamAmin ShivamAmin changed the title feature/book reader swipe pagination feat: book reader swipe pagination Feb 29, 2024
@ShivamAmin ShivamAmin marked this pull request as draft February 29, 2024 16:32
@ShivamAmin
Copy link
Contributor Author

ShivamAmin commented Feb 29, 2024

I added the toggle for this feature to the user preferences page, but for it to work we need to add the migration. I've done this before, but I can't figure out how it's done in this stack. I'm happy to read documentation and add it myself

@majora2007
Copy link
Member

To add the migration you add the setting to the user preferences entity then type dotnet ef migrations add MigrationNameHere.

Review the SQL and launch Kavita to have it applied.

@ShivamAmin
Copy link
Contributor Author

ShivamAmin commented Feb 29, 2024

To add the migration you add the setting to the user preferences entity then type dotnet ef migrations add MigrationNameHere.

Review the SQL and launch Kavita to have it applied.

Great, I'll also see about adding this to the readme or somewhere so it's documented, I'm sure I'll need to reference it in the future.

@majora2007
Copy link
Member

Just an FYI, I am still planning to review this. I have all bugfixes and PRs on hold until I wrap this comic rework. I'm really curious to see how this works and go through the code.

@majora2007
Copy link
Member

Hey, just curious if you're up for fixing merge conflicts? I have wrapped the comic rework stuff and there is time to review this and see if it can make the v0.8 release.

@ShivamAmin ShivamAmin force-pushed the feature/book-reader-swipe-pagination branch from 698f16d to 7937dd2 Compare April 6, 2024 23:56
@ShivamAmin ShivamAmin force-pushed the feature/book-reader-swipe-pagination branch from 7937dd2 to 2d70a29 Compare April 7, 2024 00:01
Copy link

sonarcloud bot commented Apr 7, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ShivamAmin
Copy link
Contributor Author

Hey, just curious if you're up for fixing merge conflicts? I have wrapped the comic rework stuff and there is time to review this and see if it can make the v0.8 release.

Sorry it took a while, but the merge conflicts have been resolved, and I tried to clean up the commit history

@majora2007
Copy link
Member

Thanks, I'll review the PR this week. Looks like quite a big one.

@majora2007
Copy link
Member

Hey, sorry I haven't tackled this yet. It's a big PR with a lot of different things added in (for future reference, smaller PRs that are directed with one feature is much better for me).

It's in my mind, but not a priority until I have enough time to really tackle and test.

Copy link
Member

@majora2007 majora2007 left a comment

Choose a reason for hiding this comment

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

Some initial testing:

  • Overflow issue on the tooltip and tooltip is on second line
    image

  • Doesn't seem like I can trigger it on default settings with dev tools responsive mode (which works for my implementation of swipe to paginate on the manga reader). Can you provide guidance?

  • I like this design, but we probably want a reset to default button on the right side to reset if we changed it.
    image

  • Immersive mode is no longer working. I have it enabled with swipe pagination but the action bars are still visible

  • Swipe should be disabled on Non-column mode in my opinion

  • Swipe mode (and Tap to paginate) should be enabled on non-immersive mode. Not sure why you changed this.

  • Since I can't test swipe pagination I cannot confirm, but did you test this with vertical writing mode?

  • Tap pagination is completely broke as well, so I can't test the fix with the menu popping.

I did not review the code yet. But based on initial surface level testing, this def needs a lot of work.

I would recommend doing this into pieces with smaller PRs tackling one thing at a time. Much easier on me to review and test and much more likely to get changes merged sooner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
2 participants