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

Adding a default_comment_sort_type column for local_site and local_user. #4469

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Feb 24, 2024

- Renamed SortType to PostSortType in the DB and code.
- Renamed references to default_sort_type to default_post_sort_type.
- Fixes #4128
Copy link
Member Author

@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.

Most of the changes are just boilerplate for me renaming SortType -> PostSortType, and can be ignored. I commented below on the main changes.

@@ -90,8 +90,14 @@ pub struct SaveUserSettings {
pub show_scores: Option<bool>,
/// Your user's theme.
pub theme: Option<String>,
pub default_sort_type: Option<SortType>,
/// The default post listing type, usually "local"
Copy link
Member Author

Choose a reason for hiding this comment

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

Grouped these together.

@@ -166,7 +171,7 @@ pub struct GetPersonDetails {
pub person_id: Option<PersonId>,
/// Example: dessalines , or dessalines@xyz.tld
pub username: Option<String>,
pub sort: Option<SortType>,
pub sort: Option<PostSortType>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Still debating whether to separate this into post_sort and comment_sort, or to keep the current post_sort_to_comment_sort mapping.

The combined view is still limited by #2444

Copy link
Member

Choose a reason for hiding this comment

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

The mapping is necessary anyway because of search. Its pretty awkward but I dont see any good way to get rid of it.


/// Returns a default instance-level comment sort type, if none is given by the user.
/// Order is type, local user default, then site default.
fn comment_sort_type_with_default(
Copy link
Member Author

Choose a reason for hiding this comment

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

Added an identical function to the one above, but for comment sort types.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested these migrations, and they work well.

Copy link
Member

@Nutomic Nutomic left a comment

Choose a reason for hiding this comment

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

Looks good. But as this is a breaking change, we should wait at least until 0.19.4 is released before merging.

@dessalines
Copy link
Member Author

Sounds good. In that case I'll try to get on all the UI changes necessary for 0.19.4 this week.

@dessalines dessalines marked this pull request as draft March 11, 2024 02:47
@dessalines dessalines added this to the 0.20.0 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to choose default comment sort in user settings
2 participants