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
base: main
Are you sure you want to change the base?
Conversation
- Renamed SortType to PostSortType in the DB and code. - Renamed references to default_sort_type to default_post_sort_type. - Fixes #4128
There was a problem hiding this 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" |
There was a problem hiding this comment.
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>, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Sounds good. In that case I'll try to get on all the UI changes necessary for |
default_sort_type
->default_post_sort_type