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 UI setting for collapsing bot comments. Fixes #3838 #4098

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Oct 24, 2023

This is only to be used by front ends, no back-end logic changes required.

@Nutomic
Copy link
Member

Nutomic commented Oct 25, 2023

Having this as a separate column means you could set show_bot_accounts = true; collapse_bot_comments = true which doesnt make sense. Better convert it to an enum like this:

enum DisplayBotComments {
    Show,
    Collapse,
    Hide
}

Only problem is that requires a breaking change, but now we have a major version change anyway.

@dessalines
Copy link
Member Author

These are a bit different, because one is show bot accounts, which means maybe hiding their posts as well, whereas this setting is specific to bot comments.

There might be other bot-related settings we add in the future, its up to front ends to hide all other bot-related settings if the "top level one", show_bot_accounts, is false.

@Nutomic
Copy link
Member

Nutomic commented Oct 25, 2023

That would still fit into an enum without problems, like

enum BotContent {
    Show,
    CollapseComments,
    Hide
}

@dessalines
Copy link
Member Author

dessalines commented Oct 25, 2023

I'd really prefer us not trying to use unified enums for logically different switches. One is about all bot content, the other is about bot comments specifically. We ran into this before with registration modes, trying to fit everything into a single enum, when in reality email verification and captcha were logically distinct from the registration mode.

@SleeplessOne1917
Copy link
Member

Is hiding bot comments and auto-collapsing them different in the same way your registration types are? Collapsing bot comments requires that they be shown, whereas email verification and captcha can be enabled independently of each other.

@dessalines
Copy link
Member Author

Collapsing bot comments requires that they be shown,

While that's true (because they're both somewhat related to visibility), you can see how the enum rather than switch approach would fail as soon as we added anything specific to bot posts, lets say a CollapsePosts:

enum BotContent {
    Show,
    Hide,
    // This is where it gets strange
    CollapsePostsButNotComments,
    CollapseCommentsButNotPosts,
    CollapseEverything,
}

As opposed to distinct booleans for collapse_posts and collapse_comments .

@Nutomic
Copy link
Member

Nutomic commented Oct 30, 2023

What is "collapse posts" supposed to mean? Seems like a contrived example, if anything you would hide the posts and then comments can be hidden as well.

@dessalines
Copy link
Member Author

It can be any attribute that differentiates posts from comments, collapse was just something I thought up . Maybe a highlightPost vs highlightComment, those also shouldn't be represented by enums, but by separate booleans.

@Nutomic Nutomic force-pushed the autocollapse_bot_comment_setting branch from 6551ad1 to 6a95434 Compare November 2, 2023 11:54
@Nutomic Nutomic enabled auto-merge (squash) November 2, 2023 11:54
@Nutomic
Copy link
Member

Nutomic commented Nov 2, 2023

Alright approved, ci is giving random failures though.

@dessalines
Copy link
Member Author

Not sure what's causing those federation errors, but it seems to be unrelated.

@dessalines dessalines disabled auto-merge November 6, 2023 21:09
@dessalines dessalines merged commit 97a4fb9 into main Nov 6, 2023
dullbananas pushed a commit to dullbananas/lemmy that referenced this pull request Nov 7, 2023
* Also order reports by oldest first (ref LemmyNet#4123) (LemmyNet#4129)

* Support signed fetch for federation (fixes LemmyNet#868) (LemmyNet#4125)

* Support signed fetch for federation (fixes LemmyNet#868)

* taplo

* add federation queue state to get_federated_instances api (LemmyNet#4104)

* add federation queue state to get_federated_instances api

* feature gate

* move retry sleep function

* move stuff around

* Add UI setting for collapsing bot comments. Fixes LemmyNet#3838 (LemmyNet#4098)

* Add UI setting for collapsing bot comments. Fixes LemmyNet#3838

* Fixing clippy check.

* Only keep sent and received activities for 7 days (fixes LemmyNet#4113, fixes LemmyNet#4110) (LemmyNet#4131)

* Only check auth secure on release mode. (LemmyNet#4127)

* Only check auth secure on release mode.

* Fixing wrong js-client.

* Adding is_debug_mode var.

* Fixing the desktop image on the README. (LemmyNet#4135)

* Delete dupes and add possibly missing unique constraint on person_aggregates.

* Fixing clippy lints.

---------

Co-authored-by: Nutomic <me@nutomic.com>
Co-authored-by: phiresky <phireskyde+git@gmail.com>
dessalines added a commit that referenced this pull request Nov 13, 2023
* post_saved

* fmt

* remove unique and not null

* put person_id first in primary key and remove index

* use post_saved.find

* change captcha_answer

* remove removal of not null

* comment_aggregates

* comment_like

* comment_saved

* aggregates

* remove "\"

* deduplicate site_aggregates

* person_post_aggregates

* community_moderator

* community_block

* community_person_ban

* custom_emoji_keyword

* federation allow/block list

* federation_queue_state

* instance_block

* local_site_rate_limit, local_user_language, login_token

* person_ban, person_block, person_follower, post_like, post_read, received_activity

* community_follower, community_language, site_language

* fmt

* image_upload

* remove unused newtypes

* remove more indexes

* use .find

* merge

* fix site_aggregates_site function

* fmt

* Primary keys dess (#17)

* Also order reports by oldest first (ref #4123) (#4129)

* Support signed fetch for federation (fixes #868) (#4125)

* Support signed fetch for federation (fixes #868)

* taplo

* add federation queue state to get_federated_instances api (#4104)

* add federation queue state to get_federated_instances api

* feature gate

* move retry sleep function

* move stuff around

* Add UI setting for collapsing bot comments. Fixes #3838 (#4098)

* Add UI setting for collapsing bot comments. Fixes #3838

* Fixing clippy check.

* Only keep sent and received activities for 7 days (fixes #4113, fixes #4110) (#4131)

* Only check auth secure on release mode. (#4127)

* Only check auth secure on release mode.

* Fixing wrong js-client.

* Adding is_debug_mode var.

* Fixing the desktop image on the README. (#4135)

* Delete dupes and add possibly missing unique constraint on person_aggregates.

* Fixing clippy lints.

---------

Co-authored-by: Nutomic <me@nutomic.com>
Co-authored-by: phiresky <phireskyde+git@gmail.com>

* fmt

* Update community_block.rs

* Update instance_block.rs

* Update person_block.rs

* Update person_block.rs

---------

Co-authored-by: Dessalines <dessalines@users.noreply.github.com>
Co-authored-by: Nutomic <me@nutomic.com>
Co-authored-by: phiresky <phireskyde+git@gmail.com>
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