Add a vote_display_mode local_user setting.

[dessalines]

• 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

[dessalines]

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

[Nutomic]

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.

[Nutomic]

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

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

[SleeplessOne1917]

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.

[dessalines]

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

1. 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.
2. Some of them conflict, especially HideAll.
3. 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.

[Nutomic]

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.

[dessalines]

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.

[Nutomic]

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?

[dessalines]

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]

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.

[MV-GH]

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

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

[Nutomic]

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]

Lemmynade is using:

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

[dessalines]

Seems like those map to:

hideall, full, score, and upvotepercentage.

[MV-GH]

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]

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

[MV-GH]

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]

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.

[dessalines]

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.

[Nutomic]

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.

[dessalines]

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.

[Nutomic]

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

[dessalines]

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.

[dessalines]

The UserSettings backup changes are included in this PR.

[Nutomic]

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.

[dessalines]

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.

[Nutomic]

Alright. This can be merged then?

[Nutomic]

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

[dessalines]

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