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 swiping between posts in PostActivity #872

Closed
wants to merge 40 commits into from
Closed

Add swiping between posts in PostActivity #872

wants to merge 40 commits into from

Conversation

kensand
Copy link

@kensand kensand commented Jun 24, 2023

This was a feature that I absolutely could not live without in BaconReader. This lets you swipe left and right when in a post to get to the next or previous one. Let me know your thoughts.

Thanks!

@MV-GH
Copy link
Collaborator

MV-GH commented Jun 24, 2023

Does this conflict with #785 ?

@kensand
Copy link
Author

kensand commented Jun 24, 2023

Huh - To be honest, I didn't even notice that feature. I suppose this renders that non-functional - I'm using my build right now on my phone and it does not go back to the HomeActivity when swiping right.

I guess my question then is how to best bring the two features together? Personally, I can live without #785 in favor of this.

@MV-GH
Copy link
Collaborator

MV-GH commented Jun 24, 2023

The codeowners are gonna have to decide that but most likely you will have to set an option somewhere to decide between yours, the previous and none.

@kensand
Copy link
Author

kensand commented Jun 24, 2023

Yeah, that's probably the best solution - I'll get started on adding that as an option in the meantime.

Copy link
Contributor

@twizmwazin twizmwazin left a comment

Choose a reason for hiding this comment

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

Let's make this a setting, "Allow swiping between posts" or something like that. Disabled by default. When disabled, the current behaviour is maintained where swiping right on the post view will return you to the home/community view. If enabled, then you must use back or the back arrow in the top left to navigate back, and swiping left/right switches between posts.

@kensand
Copy link
Author

kensand commented Jun 25, 2023

Cleaned everything up a bit and got it functioning as requested @twizmwazin - should be all set for your review 🤞

Thanks for taking the time!

@nahwneeth
Copy link
Contributor

What behaviour should be expected when the post is opened via deep link? The post might not be in the list of posts loaded by HomeViewModel.

@kensand
Copy link
Author

kensand commented Jun 26, 2023

@nahwneeth If the current post is not in the post list, then swiping will simply have no effect; there is no forward/backward on a deep link, right?

@kensand kensand requested a review from twizmwazin July 1, 2023 14:32
@MV-GH
Copy link
Collaborator

MV-GH commented Jul 7, 2023

@twizmwazin imo there should also be an option to disable both behaviours, i frequent /c/jerboa and #979. There are users who do not want either. Simple turning it into a Enum with a additional None would suffice and it should default to none.

@kensand
Copy link
Author

kensand commented Jul 9, 2023

@twizmwazin imo there should also be an option to disable both behaviours, i frequent /c/jerboa and #979. There are users who do not want either. Simple turning it into a Enum with a additional None would suffice and it should default to none.

FWIW, I agree, and am happy to implement it that way 👍

kensand added 3 commits July 9, 2023 12:56
# Conflicts:
#	app/src/main/java/com/jerboa/MainActivity.kt
#	app/src/main/java/com/jerboa/db/AppDB.kt
#	app/src/main/java/com/jerboa/model/CommunityViewModel.kt
#	app/src/main/java/com/jerboa/model/HomeViewModel.kt
#	app/src/main/java/com/jerboa/ui/components/post/PostActivity.kt
# Conflicts:
#	app/schemas/com.jerboa.db.AppDB/18.json
#	app/src/main/java/com/jerboa/db/AppDB.kt
#	app/src/main/java/com/jerboa/db/Migrations.kt
var communityName: String? by mutableStateOf(null)

var fetchingMore by mutableStateOf(false)
private set
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to re-merge from main, because all these should be removed now.

@kensand kensand requested a review from MV-GH as a code owner September 17, 2023 12:21
Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

Thanks for coming back to this. I ll take a deeper look some time later

app/build.gradle.kts Outdated Show resolved Hide resolved
app/schemas/com.jerboa.db.AppDB/18.json Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/db/AppDB.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/model/CommunityViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/model/CommunityViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/model/CommunityViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/ui/components/common/Route.kt Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/model/CommunityViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/model/HomeViewModel.kt Outdated Show resolved Hide resolved
} else {
appState.toPost(id = crv.post.id)
}
// TODO navigate to comment (using appState.toComment()) once the route is supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's blocking this?

Copy link
Author

Choose a reason for hiding this comment

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

In my testing, navigating to the Comment route caused a crash, presumably because it is not supported by any existing composable 🤷

@kensand
Copy link
Author

kensand commented Sep 18, 2023

I found a bug where, after having already gone into the CommunityActivity once, going back to the HomeActivity and then back to a different CommunityActivity, then trying to use swiping between posts no longer works, which appears to be due to the CommunityViewModel that is passed into the PostSwipeWrapper not being updated/recreated with the new communityId (and therefor not containing the current PostId). I'll need to have a further look at that.

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

There also some warnings that need to fixed. I still need to take a deeper look at this

showVotingArrowsInListView = appSettings.showVotingArrowsInListView,
showParentCommentNavigationButtons = appSettings.showParentCommentNavigationButtons,
navigateParentCommentsWithVolumeButtons = appSettings.navigateParentCommentsWithVolumeButtons,
siteViewModel = siteViewModel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the toComment crashes because you removed this composable.

This is the one that makes it possible

@dessalines
Copy link
Member

There are so many changes here that you might be better off starting from main and either copy-pasting or cherry-picking.

@dessalines
Copy link
Member

Stale PR. I can re-open if necessary.

@dessalines dessalines closed this Sep 26, 2023
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.

5 participants