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

Swipe post/comment to upvote/downvote/reply/save #1327

Merged
merged 25 commits into from
Feb 5, 2024

Conversation

Snow4DV
Copy link
Contributor

@Snow4DV Snow4DV commented Jan 21, 2024

This feature will let user swipe to upvote/downvote posts/comments

Closes feature request #52

Currently it is WIP (implemented the composable that will wrap post/comment composables)

As default probably it would be better to set it up as "swipe to start - upvote, swipe to end - downvote" but this will affect "swipe to go back/to open drawer" gesture

Thinking about customization - how should it be implemented? Doing it straight-forward will add a lot of stuff to the settings entity (that is stored in room).

There are multiple ways:

  1. Do a bunch of presets (it will limit the customization but it is the easiest one that won't affect the current settings structure a lot)
  2. Create separate object for gestures preferences and convert it using the TypeConverter to json (with gson) when storing in room (4 lists of swipeactiontypes)
    4 settings will be added (left/right gesture actions for posts/comments)
  3. You tell me :)

298275487-399f8213-c705-42d6-93c6-fcb65018ce8e

@dessalines
Copy link
Member

dessalines commented Jan 21, 2024

I wouldn't mind following the standard (I think @aeharding has with voyager):

  • Swipe right: Upvote, then downvote
  • Swipe left: Reply, then save/bookmark.

As far as customization, I really don't think we should add or try to maintain custom sets of actions. That would be immensely complicated and cumbersome to maintain. A simple boolean setting for swipe actions, default true, should work.

For colors: rather than gradients and fading, I'd just do static colors. And I'd much prefer using the already existing Material v3 colors: primary, secondary, tertiary. Make sure upvote and downvote colors match what those colors already are.

@MV-GH
Copy link
Collaborator

MV-GH commented Jan 22, 2024

For custom actions I do have something proposed see #1318

As far as customization, I really don't think we should add or try to maintain custom sets of actions.

Am I missing something but too it seems we can achieve this rather simple with one enum describing the actions and have in appSettings these additional settings

val swipeLeftPostAction: Int,
val swipeRightPostAction: Int,
val tapCommentAction: Int,
val longTapCommentAction: Int,

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 22, 2024

For custom actions I do have something proposed see #1318

As far as customization, I really don't think we should add or try to maintain custom sets of actions.

Am I missing something but too it seems we can achieve this rather simple with one enum describing the actions and have in appSettings these additional settings

val swipeLeftPostAction: Int,
val swipeRightPostAction: Int,
val tapCommentAction: Int,
val longTapCommentAction: Int,

More like 4 enums :)
one for short left swipe and one for long left swipe, same thing for right ones

Might also be good to add ability to keep all swipes on right side

I think it's not that important feature to make it fully-customizable. I might just create four presets (all actions on the left - all actions on the right - votes left, reply and save right - votes right, reply and save left. Easy to implement/maintain, will be enough for 99% of users

@MV-GH
Copy link
Collaborator

MV-GH commented Jan 22, 2024

I saw it more as one enum with all these actions
image
that has different triggers

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 22, 2024

I saw it more as one enum with all these actions
image
that has different triggers

Made a mistake in my previous comment, i meant four variables that would store enum.

That approach won't let you add four actions to one side (honestly i got used to this one because of sync) but might be more flexible to someone. idk then, maybe your approach is better, but won't matter to end user much any way :)

@MV-GH
Copy link
Collaborator

MV-GH commented Jan 22, 2024

You mean like trigger that does both upvote and save?

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 22, 2024

You mean like trigger that does both upvote and save?

That's how I see it:

Screenshot

Screenshot_20240123-003416

@Snow4DV Snow4DV marked this pull request as ready for review January 22, 2024 21:49
@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 22, 2024

I wouldn't mind following the standard (I think @aeharding has with voyager):

* Swipe right: Upvote, then downvote

* Swipe left: Reply, then save/bookmark.

As far as customization, I really don't think we should add or try to maintain custom sets of actions. That would be immensely complicated and cumbersome to maintain. A simple boolean setting for swipe actions, default true, should work.

For colors: rather than gradients and fading, I'd just do static colors. And I'd much prefer using the already existing Material v3 colors: primary, secondary, tertiary. Make sure upvote and downvote colors match what those colors already are.

Used these colors for actions:

Upvote -> MaterialTheme.colorScheme.primary
Downvote -> MaterialTheme.colorScheme.tertiary
Reply -> MaterialTheme.colorScheme.onPrimary
Save -> MaterialTheme.colorScheme.onTertiary

About colors' transitions: there were no gradients but i used animateColorAsState to achieve smooth transition between two colors. Made it fast enough so it's not notable but improves overall experience

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

I'll do a more thorough review of this later, but my biggest concerns are moving instant voting state out of the post listing. You could avoid that by passing that info up and down as needed.

Also try not to use Int for that settings preset, when you really want the Enum.

@@ -39,10 +39,11 @@ val APP_SETTINGS_DEFAULT =
showPostLinkPreviews = true,
postActionbarMode = 0,
autoPlayGifs = false,
swipeToActionPreset = 1,
Copy link
Member

Choose a reason for hiding this comment

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

Use your setting Enum, and .ordinal .


companion object {
fun getActionToRangeList(actions: List<SwipeToActionType>): List<Pair<OpenEndRange<Float>, SwipeToActionType>> {
val delta = if (actions.size > 2) 0.14f else 0.18f
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if here is where the issue is, but it switches from Upvote to Downvote way too quickly. Each action needs a longer swipe range (especially the first).

@@ -295,6 +298,17 @@ fun PostActivity(
.testTag("jerboa:comments"),
) {
item(key = "${postView.post.id}_listing", "post_listing") {
val instantScores =
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason these are getting moved around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the reason these are getting moved around?

Honestly that wasn't my best approach, agree to that. Would be better to pass VoteType to child composables

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 23, 2024

I'll do a more thorough review of this later, but my biggest concerns are moving instant voting state out of the post listing. You could avoid that by passing that info up and down as needed.

Also try not to use Int for that settings preset, when you really want the Enum.

I've spent a while thinking about a way to keep instantscores down in the composable hierarchy and the only one that comes to my mind (like passing channel to the child composable) will break the states go down - events go up principle. Modifying postview right away also doesn't look very good but it is possible if we pass it as state

i'm thinking about creating state object that holds postView and InstantScore objects and stores modified postview as state that holds correct my_vote, upvotes and downvotes count but there's a problem when child composable modifies vote and parent doesn't know anything about it

@dessalines
Copy link
Member

@MV-GH would probably be able to give better guidance than I could... But I believe you could pass down the actions like swipeUpVote() -> Unit, and allow the instant scores state to be altered inside by those actions.

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 29, 2024

I see that instantscores were refactored a little, finally got time, will take a look at it and improve according to new implementation.

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Jan 29, 2024

@MV-GH would probably be able to give better guidance than I could... But I believe you could pass down the actions like swipeUpVote() -> Unit, and allow the instant scores state to be altered inside by those actions.

Reimplemented it another way: just nested it inside single post's composable, should be fine, instantscore lays where it should now. Fixed the ranges also, you may have a look when you have time

@MV-GH
Copy link
Collaborator

MV-GH commented Jan 30, 2024

I'll review it friday if it hasn't been reviewed by then.

@MV-GH
Copy link
Collaborator

MV-GH commented Feb 2, 2024

@dessalines also should this be enabled by default? I'll let you decide

object : Migration(27, 26) {
override fun migrate(db: SupportSQLiteDatabase) {
db.execSQL(
"ALTER TABLE AppSettings DROP COLUMN swipe_to_action_preset",
Copy link
Collaborator

Choose a reason for hiding this comment

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

android.database.sqlite.SQLiteException: near "DROP": syntax error (code 1 SQLITE_ERROR): , while compiling: ALTER TABLE AppSettings DROP COLUMN swipe_to_action_preset
                                                                                                    	at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
                                                                                                    	at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1045)

It seems that DROP operations aren't supported

@dessalines should we remove all these downgrade migrations? They don't work anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh. It's a surprise to me that SQLite doesn't support drop column in tables. It can be done via creating a separate table without that column (i looked into it and i guess that's how others do it)

I think separate PR should be created for downgrade migrations fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah probably, but the down migrations aren't really used. So it won't be priority

Copy link
Member

Choose a reason for hiding this comment

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

Yeah its totally fine to remove them. One of the worst android bugs in existence: https://stackoverflow.com/questions/8045249/how-do-i-delete-column-from-sqlite-table-in-android

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Feb 4, 2024

Have a look at resolved comments. Fixed everything & also improved behaviour when account is not logged in (now it doesn't update instanceScore and still shows toast as intended).

About the latest comment: I think not working migrations should be left now so this could be improved in another pull request. This can be done by creating a separate table without newer column (see this)

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.

Other then that one remark its fine

@MV-GH
Copy link
Collaborator

MV-GH commented Feb 4, 2024

for light theme, the white icon doesn't really work. You could add as black shim or use the color from the theme

studio64_4YKa6RD9e5.mp4

@dessalines
Copy link
Member

Use one of the theme background colors, probably surface.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Thx! Sry about my delay, I should be able to test again tmrw.

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Feb 5, 2024

Use one of the theme background colors, probably surface.

Surface color is already used by surface, so had to use something else. Had a problem with color collisions (error is tertiary when the theme is dark but reddish when it is light) so i used these colors to make them different and readable on light/dark themes:

@Composable
    fun getActionColor(): Color {
        return when (this) {
            Upvote -> MaterialTheme.colorScheme.secondary
            Downvote -> MaterialTheme.colorScheme.error
            Reply -> MaterialTheme.colorScheme.inversePrimary
            Save -> MaterialTheme.colorScheme.primary
        }
    }

Now upvote/downvote colors also match in light theme (didn't match previously)

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Feb 5, 2024

for light theme, the white icon doesn't really work. You could add as black shim or use the color from the theme
studio64_4YKa6RD9e5.mp4

I think white icons look good when they're placed on proper accent colors. Should be fine now (fixed theming when using both light/dark themes)

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Just tested, and it works really well! The colors look good to me, but we can always tweak them later.

The only issue I found: bookmarking / saving only works to save, but not to unsave it, unlike the button version.

You want to handle the conflicts, or should I?

@Snow4DV
Copy link
Contributor Author

Snow4DV commented Feb 5, 2024

Just tested, and it works really well! The colors look good to me, but we can always tweak them later.

The only issue I found: bookmarking / saving only works to save, but not to unsave it, unlike the button version.

You want to handle the conflicts, or should I?

Totally missed it, happened due to lambda caching (it kept old instance of postView). Fixed that thing so now everything works correct

I resolved all conflicts so should be fine to merge now

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Sweet, thx!

@dessalines dessalines enabled auto-merge (squash) February 5, 2024 19:08
@dessalines dessalines merged commit fd42bb8 into LemmyNet:main Feb 5, 2024
1 check passed
dessalines added a commit that referenced this pull request Feb 7, 2024
* SwipeToAction composable

* Swipe to downvote/upvote feature implemented with preset-based customization

* Fix kotlin format

* fix string resource

* Do not use SwipeToAction when it is disabled

* Improve scrolling experience

* Increase color shift animation speed

* Improve ranges

* new preset: only votes

* fix deltas & rename resources

* SwipeToAction implemented correctly, SwipeToDismiss replaced with SwipeToDismissBox, fixed swipe ranges

* Kotlin format

* use ordinal of enum in AppDB instead of int

* Fixed behaviour when downvotes disables/when not logged in

* Fix formatting

* Remove default param for enableDownVotes in SwipeToAction

* Fix colors for swipe actions

* Fixed lambda caching in rememberSwipeActionState

* Format kotlin

---------

Co-authored-by: Dessalines <[email protected]>
dessalines added a commit that referenced this pull request Feb 12, 2024
* Starting to work on registration applications.

* Remove ImmutableList and add stability config file (#1342)

* Adding admin / mod view votes. (#1331)

* Starting to work on admin_view_votes, needs API added yet.

* Finished up adding admin view votes.

* Fix package name.

* Upping lemmy-api version.

* Adding a moderation subfield to post and comment action dropdowns.

* Adding feature flag check.

* Addressing PR comments.

- Moving padding up to Box.
- Using alternate feature flag method.

* Temp workaround for compose-settings height bug. (#1345)

- See alorma/Compose-Settings#203
- Fixes #1338

* Swipe post/comment to upvote/downvote/reply/save (#1327)

* SwipeToAction composable

* Swipe to downvote/upvote feature implemented with preset-based customization

* Fix kotlin format

* fix string resource

* Do not use SwipeToAction when it is disabled

* Improve scrolling experience

* Increase color shift animation speed

* Improve ranges

* new preset: only votes

* fix deltas & rename resources

* SwipeToAction implemented correctly, SwipeToDismiss replaced with SwipeToDismissBox, fixed swipe ranges

* Kotlin format

* use ordinal of enum in AppDB instead of int

* Fixed behaviour when downvotes disables/when not logged in

* Fix formatting

* Remove default param for enableDownVotes in SwipeToAction

* Fix colors for swipe actions

* Fixed lambda caching in rememberSwipeActionState

* Format kotlin

---------

Co-authored-by: Dessalines <[email protected]>

* Almost finished viewmodel.

* A few more additions.

* Finishing up registration applications.

* Adding a locally generated changelog.

- Adds a last_version_code_viewed, that gets updated in the DB,
  and compared against the current version to show the changelog.
  This means we never have to manually update that column again.
- Add a generate_changelog.sh script that uses git-cliff. It copies
  the changelog into code assets, which can be done before the release.
- Fixes #1272

* Removing 27 schema

* Removing 27 schema 2

* Moving changelog fetching to viewModel.

* Persisting admin and mod status to DB.

* Fixing format.

* Removing weird changes.

* Don't display bottomNav until acc loads

* Fix formatting

* Fix swapToAnon invalidating jwt + fix navbar not showing for anon

---------

Co-authored-by: Maarten Vercruysse <[email protected]>
Co-authored-by: Mikhail Loginov <[email protected]>
Co-authored-by: maarten.vercruysse <[email protected]>
dessalines added a commit that referenced this pull request Feb 14, 2024
* Starting to work on registration applications.

* Remove ImmutableList and add stability config file (#1342)

* Adding admin / mod view votes. (#1331)

* Starting to work on admin_view_votes, needs API added yet.

* Finished up adding admin view votes.

* Fix package name.

* Upping lemmy-api version.

* Adding a moderation subfield to post and comment action dropdowns.

* Adding feature flag check.

* Addressing PR comments.

- Moving padding up to Box.
- Using alternate feature flag method.

* Temp workaround for compose-settings height bug. (#1345)

- See alorma/Compose-Settings#203
- Fixes #1338

* Swipe post/comment to upvote/downvote/reply/save (#1327)

* SwipeToAction composable

* Swipe to downvote/upvote feature implemented with preset-based customization

* Fix kotlin format

* fix string resource

* Do not use SwipeToAction when it is disabled

* Improve scrolling experience

* Increase color shift animation speed

* Improve ranges

* new preset: only votes

* fix deltas & rename resources

* SwipeToAction implemented correctly, SwipeToDismiss replaced with SwipeToDismissBox, fixed swipe ranges

* Kotlin format

* use ordinal of enum in AppDB instead of int

* Fixed behaviour when downvotes disables/when not logged in

* Fix formatting

* Remove default param for enableDownVotes in SwipeToAction

* Fix colors for swipe actions

* Fixed lambda caching in rememberSwipeActionState

* Format kotlin

---------

Co-authored-by: Dessalines <[email protected]>

* Almost finished viewmodel.

* A few more additions.

* Finishing up registration applications.

* Adding a locally generated changelog.

- Adds a last_version_code_viewed, that gets updated in the DB,
  and compared against the current version to show the changelog.
  This means we never have to manually update that column again.
- Add a generate_changelog.sh script that uses git-cliff. It copies
  the changelog into code assets, which can be done before the release.
- Fixes #1272

* Removing 27 schema

* Removing 27 schema 2

* Adding the admin / mod report queue.

* Moving changelog fetching to viewModel.

* Persisting admin and mod status to DB.

* Fixing format.

* Removing weird changes.

* Missed formatting.

* Forgot to remove unused block.

* Fixing missing fullBody.

* Addressing PR comments.

---------

Co-authored-by: Maarten Vercruysse <[email protected]>
Co-authored-by: Mikhail Loginov <[email protected]>
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.

3 participants