[WIP] Save the last Sort used per subreddit #15
Conversation
| @@ -35,6 +36,58 @@ public static SortingAndTimePeriod create(SubredditSort sortOrder, TimePeriod ti | |||
| return new AutoValue_SortingAndTimePeriod(sortOrder, timePeriod); | |||
| } | |||
|
|
|||
| public static SortingAndTimePeriod create(String sortOrderTimePeriod) { | |||
| String[] splittedString = sortOrderTimePeriod.split(SEPARATOR); | |||
There was a problem hiding this comment.
It might be a better idea to use Moshi for serializing and deserializing SortingAndTimePeriod. Doing it by hand is prone to errors and breakage in case the schema changes in the future.
There was a problem hiding this comment.
I need to read more about Moshi. I've tried to use it in another feature with no luck. 👍
There was a problem hiding this comment.
I'll gladly walk you through Moshi if you want
| @@ -45,6 +61,39 @@ public Completable logout() { | |||
| .andThen(Completable.fromAction(() -> removeLoggedInUsername())); | |||
| } | |||
|
|
|||
| public SortingAndTimePeriod getSortingAndTimePeriodForSub(String subredditName) { | |||
There was a problem hiding this comment.
We can avoid using get prefix for getters. Just sortingAndTimePeriodForSub() should be sufficient.
There was a problem hiding this comment.
Maybe lastUsedSortingAndTimePeriod() would be a better name
| @@ -22,20 +32,26 @@ | |||
| public class UserSessionRepository { | |||
There was a problem hiding this comment.
I am not sure if UserSessionRepository is the best place to store this information. This class is meant to only manage the logged in user's information and nothing else.
I wonder if we should store it in SubredditActivity directly.
There was a problem hiding this comment.
Yeah, we could store in that Activity. Honestly I have no idea how to persist data on Android Apps.
| } catch (JSONException exception) { | ||
| // Do something with exception | ||
| } | ||
| } |
There was a problem hiding this comment.
This project uses Moshi for working with JSON :)
| @@ -239,11 +241,16 @@ protected void onPostCreate(@Nullable Bundle savedState) { | |||
| //noinspection ConstantConditions | |||
| sortingChangesStream.accept(savedState.getParcelable(KEY_SORTING_AND_TIME_PERIOD)); | |||
| } else { | |||
| sortingChangesStream.accept(SortingAndTimePeriod.create(Reddit.Companion.getDEFAULT_SUBREDDIT_SORT())); | |||
| //load the last sort for this user/subreddit | |||
| this.loadLastSortingAndPeriod(subredditChangesStream.getValue()); | |||
| sortingChangesStream | ||
| .takeUntil(lifecycle().onDestroy()) | ||
| .subscribe(sortingAndTimePeriod -> { | ||
| // save this sort and period |
There was a problem hiding this comment.
The function name is self explanatory. No need for this comment.
| @@ -425,6 +432,14 @@ private void setupToolbarSheet() { | |||
| }); | |||
| } | |||
|
|
|||
| private void loadLastSortingAndPeriod(String subredditName) { | |||
| SortingAndTimePeriod sort = userSessionRepository.get().getSortingAndTimePeriodForSub(subredditName); | |||
| if (sort == null) | |||
There was a problem hiding this comment.
Braces with if and else please?
I'll soon add some static analyzer to the project so that code formatting comments do not leak into code reviews.
| @@ -491,6 +506,7 @@ private void setupSubmissionRecyclerView(@Nullable Bundle savedState) { | |||
| subredditChangesStream | |||
| .observeOn(mainThread()) | |||
| .takeUntil(lifecycle().onDestroy()) | |||
| .doAfterNext(subreddit -> loadLastSortingAndPeriod(subreddit)) | |||
There was a problem hiding this comment.
This looks weird. This Rx chain is supposed to consume changes to the subreddit, but if I understand correctly, your intention is to start with the last used sorting right?
You probably want to call loadLastSortingAndPeriod() in line #240 instead. Something along the lines of:
if (savedState != null && savedState.containsKey(KEY_SORTING_AND_TIME_PERIOD)) {
//noinspection ConstantConditions
sortingChangesStream.accept(savedState.getParcelable(KEY_SORTING_AND_TIME_PERIOD));
} else {
SortingAndTimePeriod lastUsed = loadLastSortingAndPeriod();
if (lastUsed != null) {
sortingChangesStream.accept(lastUsed);
} else {
sortingChangesStream.accept(SortingAndTimePeriod.create(Reddit.Companion.getDEFAULT_SUBREDDIT_SORT()));
}
}There was a problem hiding this comment.
For what I've test subredditChancesStream fires when a new subreddit is loaded into view. So I get the last sort used for that subreddit.
|
|
||
| @Inject | ||
| public UserSessionRepository(Lazy<Reddit> reddit, @Named("user_session") RxSharedPreferences rxSharedPreferences) { | ||
| this.reddit = reddit; | ||
| loggedInUsername = rxSharedPreferences.getString(KEY_LOGGED_IN_USERNAME, EMPTY); | ||
| sortingAndPeriod = rxSharedPreferences.getString(KEY_SORTING_AND_PERIOD, EMPTY); |
There was a problem hiding this comment.
SharedPreferences are supposed to be used for tiny key value pairs. Storing a big HashMap like this one might not be a good idea. It'll grow over time. Especially if we want to start storing the last used sorting for comments as well.
SharedPreference files are read on the main thread during app startup. As the file grows, the penalty will also grow. I think we should think of creating a separate repository class that will handle retaining this data asynchronously.
mvietri
changed the title
|
@Esselans Same question as for the other closed pull request. |
|
@Tunous I'm ditching this PR for the time being |
Both Sort And Period are saved. The next time you visit a subreddit it loads the last sort used. If no sort is available it defaults to BEST.
Note I'm fairly new to all this android programming. I know this can be done better.