Added in swipe gestures to navigate pages #383
Conversation
…lling. Reading a file while zoomed in does not cause unnecessary page changes.
|
While this is still pending, I will continue to test this build and fix any other issues with the implementation. |
thestinger
force-pushed
the
main
branch
13 times, most recently
from
731a863 to
0d0cf10
Compare
thestinger
force-pushed
the
main
branch
2 times, most recently
from
be232f1 to
799b483
Compare
thestinger
force-pushed
the
main
branch
4 times, most recently
from
fbe15bf to
99736fc
Compare
|
This seems to be aiming at the same thing as #282, and both are in the queue for a while. What needs to happen for either of them to be merged? More testing? Anything else? Moving between pages using gestures would be a great UX enhancement (for me, anyway). |
|
Yes, when I started this I didn’t realise that there was another PR doing the same thing. I would like to see this merged but I haven’t heard anything more on it. I think it needs some work to get it up to scratch with the rest of the code and would like that help. |
|
@thestinger or maybe @octocorvus (as a GOS member who had a recent-ish PR here), I'd appreciate a comment. I'm willing to help out to make this one happen, but let's clarify the blockers. @Madmatt88 could you please update the PR in the meanwhile, to trigger checks and see which ones are failing? The logs for the previous update are already gone. |
|
It needs to be implemented as swiping between pages, not a gesture across the screen that's always there. |
|
@thestinger Thank you for that piece of information, I will have a look and change the implementation to fit that requirement. @alt3r-3go I will rebase the PR and re-trigger the action to find the build error. After that I'll start to make the changes to fit the new requirements and then update the PR, if I have any major issues I will reach out. |
| @@ -24,9 +32,20 @@ static void attach(Context context, View gestureView, GestureListener listener) | |||
| final GestureDetector detector = new GestureDetector(context, | |||
| new GestureDetector.SimpleOnGestureListener() { | |||
| @Override | |||
| public boolean onSingleTapUp(MotionEvent motionEvent) { | |||
| public boolean onSingleTapUp(@NonNull MotionEvent motionEvent) { | |||
There was a problem hiding this comment.
This seems unrelated to the PR topic, I'd suggest removing, to avoid polluting git history.
| boolean onFling(@Nullable MotionEvent e1, @NonNull MotionEvent e2, float velocityX, float velocityY); | ||
| boolean onDown(); | ||
| // Can be replaced with ratio when supported | ||
| void onZoomIn(float value); |
There was a problem hiding this comment.
I wonder why would this and the next one are needed, given it's not about zooming?
| public void onZoom(float scaleFactor, float focusX, float focusY) { | ||
| zoom(scaleFactor, focusX, focusY, false); |
There was a problem hiding this comment.
Whitespace-only changes like this are usually discouraged, or at least should be split out to dedicated commits.
| float deltaY = e2.getY() - e1.getY(); | ||
|
|
||
| if (Math.abs(deltaX) > Math.abs(deltaY)){ | ||
| if ((deltaX > 0) & (Math.abs(velocityX) > maxXVelocity)) { |
There was a problem hiding this comment.
Bitwise & operator used instead of the boolean one.
| if ((deltaX > 0) & (Math.abs(velocityX) > maxXVelocity)) { | ||
| Log.d("Horizontal", "Right movement Position"); | ||
| onJumpToPageInDocument(mPage - 1); | ||
| } else if ((Math.abs(velocityX) > maxXVelocity)) { |
There was a problem hiding this comment.
I'd suggest moving this condition over to the higher-level if clause, to simplify and avoid repeating it.
| } | ||
|
|
||
| @Override | ||
| public boolean onDown() { |
|
@alt3r-3go thanks for those comments and one thing that will help this is decoupling the page change from the onFling system call. I had to add in extra conditions in there because I had big problems with the page changing while i was reading as I was moving the page from left to right. Some things I did I can’t remember fully because it has been so long ago. |
|
@alt3r-3go I have re-run the action and it gave these errors: > Task :app:compileDebugKotlin FAILED
e: file:///home/runner/work/PdfViewer/PdfViewer/app/src/main/java/app/grapheneos/pdfviewer/fragment/JumpToPageFragment.kt:49:32 Unresolved reference 'onJumpToPageInDocument'.
e: file:///home/runner/work/PdfViewer/PdfViewer/app/src/main/java/app/grapheneos/pdfviewer/fragment/PasswordPromptFragment.kt:85:37 Unresolved reference 'loadPdfWithPassword'.I am not quite sure why it can't find this reference, I have run the gradle command locally and it fails in the same section and with extra compiler flags provided other errors that didn't make sense the fix the problem. The areas that are now causing issues also were never changed so I have no idea what could cause this one. There should be no reason why it can't find the reference since the import seems correct on the files as well. Have you seen this before? Thank you for your help. |
|
Current tip of the I'd suggest you to rebase non-merge commits onto current BTW, I can take over this one if you want (with proper credit of course) and do all that myself, then tweak as needed to get it merged. |
|
So I have made one fatal mistake by not creating a new feature branch which has created a tangled web of commits and changes causing my fork to cause problems. For future work I will not be making that mistake again. I still want to keep contributing to this project and have learnt a lot by going through this PR process, if @alt3r-3go you are able to get the project to build and make the changes with ease then I am more than happy for you to that. The other option is that I will start again locally, close this PR and submit a new one that doesn't have the same headaches so it can still be added. Thanks everyone for your patience. |
Address issue #41 and adds the feature of being able to use swipe gestures to navigate the pages of a PDF.