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

[RFC] Swipe ReadingPage #468

Closed
wants to merge 5 commits into from
Closed

[RFC] Swipe ReadingPage #468

wants to merge 5 commits into from

Conversation

boun
Copy link
Contributor

@boun boun commented Sep 18, 2023

Hi,

this is just a request for comments, no proper merge request. Anyway, I'd like to "infinite scroll" from article to article, so when I reach the bottom and continue scrolling, I reach the next article. I thought about different approaches to tackle that, this is the way I got it working. So now I have a NestedScrollConnection listening to scroll and fling events, that feeds a swipeable Modifier. The connection only consumes what is left over from its child views. That should only happen when the bottom (or top) of the page is reached. Everything is implemented as a modifer, to not clutter the ReadingPage code.

I chose to have a vertical swipe, as it feels the most natural to me.

I'd appreciate a review.

What is still missing:

  • Obviously: Remove the "DO NOT MERGE" commit (it creates a debug build with a red icon)

@Ashinch
Copy link
Owner

Ashinch commented Sep 18, 2023

@boun Thank you for your explore, I've been refactoring the package structure lately and I'll spend some time going through this PR.

@Ashinch
Copy link
Owner

Ashinch commented Sep 22, 2023

Hi @boun ,

I tried it out, and it feels great. 🤩 We're already very close to the Reeder 5 experience. Now, it seems like the only thing we're missing is a gesture-based interactive animation.

I'm trying to upgrade the dependency versions in hopes that there might be some new APIs available that we can leverage during this time.

@boun
Copy link
Contributor Author

boun commented Sep 22, 2023

So I have been using it for some time now and noticed one drawback: Choosing only unread in the article list, starting reading and then swiping up/down leads to unexpected behaviour. When swiping down, the previous article is marked read and therefore vanishes from the list. Swiping up again does hence not return to that previous article but to another unread If there is even one. I'll look into that.

About the animation: you mean an arrow indicating the article change? I have looked into the swiperefresh dependency that you are using for updating the reading list but that does not work for bottom swipes. It has been replaced by something in compose that I have not tried yet (but seems to support only top swipes). So it seems like rolling an own implementation is required.

@nvllz
Copy link
Contributor

nvllz commented Oct 22, 2023

Hey! I've been testing this feature for a while, and I think it's a cool idea to make reading flow this way. But I have had numerous accidental article swipes out of nowhere. I think the swipe needs to be detected in some other way. Now you can swipe to the next article with a long swipe that starts just before the end of the article. I find this buggy, especially when it comes to the top swipe.

To possibly fix it, you could do it the other way around. Scroll only when the whole article is scrolled to the very (top or bottom) edge, and the user makes an additional swipe to load the next article.

Additionally, you may want to prevent scrolling to the previous/next article until the current article is loaded.

I managed to record some of the bugs..

xx.mp4
xxx.mp4

Thanks for your work!!

@boun
Copy link
Contributor Author

boun commented Oct 24, 2023

hi nvllz, thanks for the testing! indeed there were some glitches in the experience, but seems like @aeghn got that covered

@boun
Copy link
Contributor Author

boun commented Oct 24, 2023

closing, superseded by #481

@boun boun closed this Oct 24, 2023
@boun
Copy link
Contributor Author

boun commented Oct 25, 2023

o.k. did not read the whole thread in #481 so they wait for this branch to be merged ;-) I'll check what was done there and try to grab some time to get that to a decent state.

@boun boun reopened this Oct 25, 2023
@aeghn
Copy link
Contributor

aeghn commented Oct 25, 2023

@boun Sir, I have no intention of stealing your work. Thank you for your efforts.

I initially implemented my own version using pointerInput. However, while browsing the pull request list, I came across your implementation, which I found to be more elegant (I just started learning Kotlin two weeks ago). So, I decided to rewrite my implementation based on yours. But there are some differences between mine and yours. I personally find left and right swiping more comfortable (some RSS articles can be quite long, and I might lose interest after reading the title, so left and right swiping aligns better with my preferences). Additionally, I believe that swiping down to access the full article is more convenient in the context of larger screens. However, I also support giving users the choice to select their preferred interaction method.

Perhaps you could take a look at the code and provide some feedback. I think there are some areas that could be improved, but I haven't figured out how to handle them yet. I would greatly appreciate your help if you're willing.

I apologize for any inconvenience caused to you.

@boun
Copy link
Contributor Author

boun commented Oct 25, 2023

@aeghn Easy, no problem at all. My open issues on the list were that swiping (no matter left/right or up/down) would mark the articles read, which in turn in "unread mode" would prevent swiping back there. Fixing that would require some significant changes in the way the model works (well or a quick dirty hack). So I was waiting for opinions from others (e.g. @Ashinch) here. Plus the problems reported by nvllz. The branch is an experimental playground to explore what works best, so feel free to take and improve, I dont have that much time on my hand right now.

@Ashinch
Copy link
Owner

Ashinch commented Oct 26, 2023

I will be checking on this after the version 0.9.9 release.

@Ashinch
Copy link
Owner

Ashinch commented Jan 21, 2024

@boun

Can you provide a new switch option in Settings -> Color & Style -> Reading Page -> General (Horizontal Swipe to Next) to enable it? I think it should be off by default.

@boun
Copy link
Contributor Author

boun commented Jan 25, 2024

Yup, will do

@Ashinch Ashinch changed the base branch from main to dev February 3, 2024 15:45
@Ashinch Ashinch linked an issue Feb 5, 2024 that may be closed by this pull request
@Ashinch Ashinch changed the base branch from dev to main February 6, 2024 17:37
@boun
Copy link
Contributor Author

boun commented Feb 7, 2024

Superseeded

@boun boun closed this Feb 7, 2024
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.

Swipe left and right to switch articles
4 participants