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

Remember selected sorting options #209

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

emmavray
Copy link
Contributor

@emmavray emmavray commented Jun 20, 2023

Checklist

  • I have described what this PR contains

Choose one of the following two options:

    • This PR does not introduce major changes
    • This PR introduces major changes, and I have consulted @buresdv, @jboi or @mormaer in the Mlem Development Matrix room

Choose one of the following two options:

    • This PR does not change the UI in any way
    • This PR adds new UI elements / changes the UI, and I have attached pictures or videos of the new / changed UI elements

Pull Request Information

About this Pull Request

This is a simple change to persist across app launches, using AppStorage:

  • the currently selected SortingOptions for the feed and
  • the currently selected FeedType (All Posts, Subscribed, etc)

Before this PR, each app launch will result in the feed resetting to the default Subscribed feed sorted by Hot. After this PR, the app will remember your last selected FeedType and SortingOptions.

Screenshots and Videos

There are no UI changes, it's completely transparent to the user.

Additional Context

This change was inspired by the last few days of using Mlem. As a new Lemmy user who hasn't subscribed to many communities, I prefer to browse the All Posts feed. The default Hot sort is not very useful in this case, as the content is essentially unchanged for the last several days. Remembering my last selected FeedType and SortingOptions lets me get right into the app and start browsing.

I considered adding a sort preference in Settings (to match the Default Comment Sort option that already exists). However I hesitate to add two different UIs for a single preference; it seems to me that allowing the user to pick their preferred sort in context makes a lot of sense, and it's reasonable to expect that the app would remember this preference. Happy to adjust the implementation if an explicit setting is preferred.

Similar for FeedType -- it just makes sense that the app would remember which feed you browse.

A note on NSUserDefault keys: it may be useful to consider scoping the preferences a bit more specifically. For example in the future it may be best to persist sort options (for example) for each applicable screen; user feeds may have a different preferred sort than the post feed. Or taking it a step further, users may expect All Posts to sort differently than Subscribed or Local feeds. I suggest a reverse-domain approach.

Copy link
Collaborator

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

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

IMO this should be a user setting, and this is also a UX change

@emmavray
Copy link
Contributor Author

certainly a UX change, IMO three ways to do it:

  1. Don't persist the last feed/sort options, always use Subscribed and Hot (CURRENT BEHAVIOR)
  2. Automatically persist the last used feed/sort options, and don't provide a user setting (THIS PR)
  3. Add settings UIs to pick preferred feed and sort options, maybe including an option for Last Used

If a user setting is required it may make sense to have the default value of Last Used

@@ -36,7 +36,7 @@ struct CommunityView: View

@FocusState var isSearchFieldFocused: Bool

@State var feedType: FeedType = .subscribed
@AppStorage("selectedFeedType") var feedType: FeedType = .subscribed
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this still work when the caller is manually setting the feed type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

following up; no, my implementation of selectedFeedType does not work correctly when manually setting the type (such as when viewing posts from a specific community rather than the main feed). In all cases it will use the value in AppStorage.

@mormaer
Copy link
Collaborator

mormaer commented Jun 20, 2023

certainly a UX change, IMO three ways to do it:

  1. Don't persist the last feed/sort options, always use Subscribed and Hot (CURRENT BEHAVIOR)
  2. Automatically persist the last used feed/sort options, and don't provide a user setting (THIS PR)
  3. Add settings UIs to pick preferred feed and sort options, maybe including an option for Last Used

If a user setting is required it may make sense to have the default value of Last Used

under Settings -> General (GeneralSettingsView) there is an option for comment sorting in there, I think it'd be best (for now) to add these additional ones alongside that as a first step?

@JakeShirley
Copy link
Member

FWIW I feel like at some point we should save this setting per feed type like Apollo. Some communities I like to sort by new, some by hot, etc.

@ShadowJonathan
Copy link
Collaborator

Heya @cdstamper, we have a new repo at https://github.com/mlemgroup/mlem, due to some shuffling around we couldn't get it migrated properly, but if you could reopen this PR there, and link it back here, then it can continue to be considered ^^

@ShadowJonathan ShadowJonathan added the needs migration Issues or PRs that still need to be migrated to the new repo label Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs migration Issues or PRs that still need to be migrated to the new repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants