Add a vote_display_mode local_user setting. #4450

dessalines · 2024-02-12T21:49:23Z

Fixes Add a vote_display_mode user setting, to show upvote percentages. #4449

This also leaves in place the show_vote_scores bool column, but not removing that for backwards-compatibility purposes. It should be removed later tho, as this would supercede it.

cc @Nutomic , @MV-GH , @SleeplessOne1917 , @phiresky

- Fixes #4449

dessalines · 2024-02-12T21:50:01Z

crates/db_schema/src/lib.rs

+#[cfg_attr(feature = "full", ts(export))]
+// TODO, in future releases, remove hide_scores, as this supercedes it.
+/// A vote-display setting that changes how votes are displayed in front ends.
+pub enum VoteDisplayMode {


If anyone can think of any others, I'd be happy to add them.

I would rather try to limit the number of options. Otherwise new apps will feel forced to implement all of them which can be a lot of work.

Instead of an enum with so many variants, it would be more concise like this:

struct VoteDisplayMode { vote_counts: bool, percentage: bool, score: bool }

Does having vote_counts enabled separately from score make sense UI wise? If there is a mix of up and down votes, the score will be the number of upvotes minus the number of downvotes. In other words, the score is derived from the vote counts.

My main issue with having a lot of bools in that setting, is that

Now we'd need to either add a lot of extra boolean columns, or extract them to another table and do a join just to get this info. Makes fetching and saving more complicated, especially in UIs. Much easier to have a single dropdown with these options in a UI.

Some of them conflict, especially HideAll.

Some cases don't make sense, like DownvoteAndUpvotePercentage

Also its not necessary for every app to support them all. They can just use a default case when switching on that enum, and don't have to do every one. I imagine a lot of apps will also ignore this entirely, but we can at least support it in lemmy-ui, and jerboa.

The bools could be stored as different bits in a single u8 variable. I dont see how it would conflict, and HideAll is the same as having all bools false. And whats the problem with DownvoteAndUpvotePercentage, it would be the same as vote_counts: true, percentage: true.

To cover all these so far, the struct would need to separate upvotes and downvotes, to be:

struct VoteDisplayMode { score: bool, upvotes: bool, downvotes: bool, upvote_percentage: bool, }

As appealing as storing them all in a single column is, it doesn't seem to be a good design pattern.. Anyways I'll have to add other linked settings tables like local_user_notifications, so I think its fine to break this out into another table, and name it local_user_vote_display_mode , with these columns explicitly named.

We've had to do that for local_site_rate_limit anyway.

I dont see anything in your link claiming that its a bad design pattern. The only problem I can imagine is that is difficult to read/modify individual booleans via sql, but we dont need that. And what do you mean by local_user_notifications?

Its the top comment on this answer, and I agree with that.

When I start working on notifications, I'm also going to split this group of local user settings into its own table: local_user_notifications. So I might as well do the same for this one.

dessalines · 2024-02-12T21:53:17Z

migrations/2024-02-12-211114_add_vote_display_mode_setting/up.sql

+);
+
+ALTER TABLE local_user
+    ADD COLUMN vote_display_mode vote_display_mode_enum DEFAULT 'ScoreAndUpvotePercentage' NOT NULL;


From jerboa:

You can see that there's a lot of redundant and unecessary info in displaying all these three pieces of info, like we do now in lemmy-ui and jerboa.

I'd argue that a score and upvote pct (that only probably needs to be shown when < 90% or something), is a preferred default, as that takes it from 3 pieces of info, to 2 or 1. I see a lot of apps doing this anyway.

Also that doesn't prevent us from showing the full vote scores on click-and-hold, and of course this PR changes nothing about the API results.

(that only probably needs to be shown when < 90% or something)

I would leave this as a implementation detail for the clients to decide

crates/db_schema/src/lib.rs

Nutomic · 2024-02-13T09:59:58Z

It would be good if you could talk to devs of some different apps/frontends and to instance admins, to find out which particular options they are interested in.

silasabbott · 2024-02-14T17:36:53Z

Lemmynade is using:

Hidden
Separate (23 up, 10 down)
Combined (13 total score)
Percentage (70%)

dessalines · 2024-02-15T23:17:14Z

Seems like those map to:

hideall, full, score, and upvotepercentage.

MV-GH · 2024-02-24T09:26:32Z

With this change to Jerboa, some users expressed that they prefer the separate votes near arrows. Maybe we can add a option for this? The clients can always chose to not honor it and default to another.

SleeplessOne1917 · 2024-02-24T12:28:27Z

Aren't the separate votes already covered with the Full type?

MV-GH · 2024-02-24T12:36:09Z

No currently in Jerboa Full shows Score + Downvotes in the header

Maybe that should be discussed instead. I'll do that over at Jerboa

Nutomic · 2024-03-04T10:54:14Z

migrations/2024-02-12-211114_add_vote_display_mode_setting/up.sql

+    upvote_percentage boolean DEFAULT TRUE NOT NULL,
+    published timestamp with time zone NOT NULL DEFAULT now(),
+    updated timestamp with time zone,
+    PRIMARY KEY (local_user_id)


Why not add this directly to the local_user table? Also whats the reason for having published and updated columns? Not even local_user has those and they dont seem to serve any purpose.

Logically, they should, but diesel has those column limits that drastically increase compile time, that you can only increase via flags. https://docs.diesel.rs/master/diesel/index.html

We're already almost at 32 columns, and we had to do a similar thing to LocalSite with breaking out LocalSiteRateLimit into its own table.

I can remove the published and updated tho.

Right, in that case I would reference the separate table only directly in the db code. Meaning get rid of LocalUserVoteDisplayMode etc or make it private, so that the other Rust code and especially the api can act like its all in one struct/table. You can even rename it something like local_user_2 as we will likely add other settings to this table later.

Its a join, so it needs to be included in LocalUserView like in this PR. It does need to be well typed as its own table.

We used to try to do things like this with sql views, but overall its not worth it. This works the same way as LocalSiteView, with local_site_rate_limit as a joined table.

I think you can do something like this:

struct LocalUser{} struct LocalUser2{} struct LocalUserWrapper{ #[serde(flatten)] local_user_1: LocalUser, #[serde(flatten)] local_user_2: LocalUser2 } struct LocalUserView { local_user: LocalUserWrapper, ... }

Then the api should return all of it directly in LocalUserView.local_user

I'd really rather not do this, it goes against the standard previously set by LocalSiteRateLimit joined to LocalSite, and might have some major issues with lemmy-js-client typings.

The UserSettings backup changes are included in this PR.

Alright but I would still use a more generic name like local_user_extra because we will definitely add more settings over time. Would be annoying if we have to rename everything then, or add another table.

There will likely be other tables, I plan to add local_user_notification as a separate table as well, so local_user_extra wouldn't be a good name for this.

Alright. This can be merged then?

Nutomic · 2024-03-04T10:55:25Z

crates/db_schema/src/source/local_user.rs

@@ -36,6 +36,7 @@ pub struct LocalUser {
  pub show_avatars: bool,
  pub send_notifications_to_email: bool,
  /// Whether to show comment / post scores.
+  // TODO now that there is a vote_display_mode, this can be gotten rid of in future releases.
  pub show_scores: bool,


You can open a draft PR to remove it, then we merge whenever we start doing breaking changes for 0.20.

K I'll make a PR for that in a min.

local_user_vote_display_mode table. - See #4450

Add a vote_display_mode local_user setting.

a628af6

- Fixes #4449

dessalines requested review from phiresky and Nutomic as code owners February 12, 2024 21:49

dessalines commented Feb 12, 2024

View reviewed changes

SleeplessOne1917 reviewed Feb 12, 2024

View reviewed changes

crates/db_schema/src/lib.rs Outdated Show resolved Hide resolved

Changing HideDownvotes to Score.

9d88b62

dessalines mentioned this pull request Feb 24, 2024

Add a Full vote display mode? And what should be the default? LemmyNet/jerboa#1400

Open

3 tasks

dessalines added 4 commits February 24, 2024 18:17

Merge remote-tracking branch 'origin/main' into vote_display_mode

04faedd

Adding ScoreAndDownvote display mode.

2a60d91

Merge remote-tracking branch 'origin/main' into vote_display_mode

b2add17

Adding upvote and downvote mode.

730035f

dessalines marked this pull request as draft February 29, 2024 02:07

dessalines added 3 commits February 29, 2024 12:12

Merge branch 'main' into vote_display_mode

3b9e7f1

Extracting vote_display_mode to another table.

b2c12ad

Fixing fmt.

fda0fbc

dessalines marked this pull request as ready for review March 1, 2024 17:11

Merge branch 'main' into vote_display_mode

d56279a

Nutomic reviewed Mar 4, 2024

View reviewed changes

dessalines marked this pull request as draft March 4, 2024 14:22

Merge remote-tracking branch 'origin/main' into vote_display_mode

8f7fd94

dessalines marked this pull request as ready for review March 4, 2024 14:29

Remove published and updated columns.

d0e25e8

dessalines added a commit that referenced this pull request Mar 4, 2024

Removing local_user.show_scores column, since its now on the

de92dfb

local_user_vote_display_mode table. - See #4450

This was referenced Mar 4, 2024

Removing local_user.show_scores column, since its now on the local_user_vote_display_mode table. #4497

Draft

Adding distinct Full Score, and ScoreAndDownvote vote display modes. LemmyNet/jerboa#1402

Merged

Nutomic approved these changes Mar 12, 2024

View reviewed changes

dessalines merged commit 15f02f0 into main Mar 13, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a vote_display_mode local_user setting. #4450

Add a vote_display_mode local_user setting. #4450

dessalines commented Feb 12, 2024 •

edited

dessalines Feb 12, 2024

Nutomic Feb 27, 2024

Nutomic Feb 27, 2024 •

edited

SleeplessOne1917 Feb 27, 2024

dessalines Feb 27, 2024 •

edited

Nutomic Feb 28, 2024

dessalines Feb 29, 2024 •

edited

Nutomic Feb 29, 2024

dessalines Feb 29, 2024

dessalines Feb 12, 2024 •

edited

MV-GH Feb 12, 2024

Nutomic commented Feb 13, 2024 •

edited

silasabbott commented Feb 14, 2024 •

edited

dessalines commented Feb 15, 2024

MV-GH commented Feb 24, 2024

SleeplessOne1917 commented Feb 24, 2024

MV-GH commented Feb 24, 2024 •

edited

Nutomic Mar 4, 2024

dessalines Mar 4, 2024 •

edited

Nutomic Mar 4, 2024

dessalines Mar 4, 2024 •

edited

Nutomic Mar 5, 2024 •

edited

dessalines Mar 5, 2024 •

edited

dessalines Mar 5, 2024

Nutomic Mar 6, 2024

dessalines Mar 8, 2024

Nutomic Mar 12, 2024

Nutomic Mar 4, 2024

dessalines Mar 4, 2024

Add a vote_display_mode local_user setting. #4450

Add a vote_display_mode local_user setting. #4450

Conversation

dessalines commented Feb 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Feb 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Feb 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Feb 13, 2024 • edited

silasabbott commented Feb 14, 2024 • edited

dessalines commented Feb 15, 2024

MV-GH commented Feb 24, 2024

SleeplessOne1917 commented Feb 24, 2024

MV-GH commented Feb 24, 2024 • edited

Choose a reason for hiding this comment

dessalines Mar 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Mar 4, 2024 • edited

Choose a reason for hiding this comment

Nutomic Mar 5, 2024 • edited

Choose a reason for hiding this comment

dessalines Mar 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines commented Feb 12, 2024 •

edited

Nutomic Feb 27, 2024 •

edited

dessalines Feb 27, 2024 •

edited

dessalines Feb 29, 2024 •

edited

dessalines Feb 12, 2024 •

edited

Nutomic commented Feb 13, 2024 •

edited

silasabbott commented Feb 14, 2024 •

edited

MV-GH commented Feb 24, 2024 •

edited

dessalines Mar 4, 2024 •

edited

dessalines Mar 4, 2024 •

edited

Nutomic Mar 5, 2024 •

edited

dessalines Mar 5, 2024 •

edited