Make MultiSelectEpisodesHelper not a singleton #1579
Conversation
|
Reminder I do find that upnext & downloads haveing a multiselect being shared quite helpful 😅 |
|
I haven't forgotten @CookieyedCodes . 🙂 If the up next queue showed whether each episode downloaded or not, would you still need to use multiselect shared across the two screens? |
|
Gud gud 😊 @mchowning , I am mainly useing it to distinguish between what podcasts are in upnext & what are not, ie if I'm deciding if something is to be added to the que & is quicker then hunt & pecking the blue icon under downloads 😅 |
There was a problem hiding this comment.
This is working well in my testing. 👍
One issue I noticed:
Earlier MultiSelectEpisodesHelper inside MainActivity was used to dismiss the multi-select toolbar from the UpNext dialog instead of dismissing the dialog itself on a back gesture. It is redundant now as both do not share multi-select state.
Video from this PR
upnext_back_nav_now.mp4
Video from the app in production
upnext_back_nav_expected.mp4
I tried to fix this behavior in these commits:
f018992
38b1711
It also fixed the back behavior for multi-select toolbar on Episode Details and bookmarks from the Files bottom sheet. We can cherry-pick the changes here or I can create a new PR if you want.
This will cause back stack issues for bottom sheet dialog fragments with multi select. That means, if a bottom sheet dialog fragment is multi selecting, then on back press, it will just dismiss the dialog rather than closing the multi-select toolbar. This is fixed in the next commit.
mchowning
force-pushed
the
fix/multiselect-shared-state
branch
from
3694267 to
11a22f2
Compare
Good catch and those changes look great. I've added them to this PR. |
|
You mentioned on Slack that you were interested in adding this to The changes I've made since your review are:
|
Description
This is a follow-up to #1316. That PR contained quite a few changes, and we found other ways to address the ANRs and the worst multi-select issues, so we didn't move forward with it. We still had quite a few multi-select issues though. In particular, in my testing I observed that multi-select state was getting shared (which would cause things like the toolbar displaying the wrong number of selected items, and other multiselect bugs).
I initially tried porting over a few of the changes from #1316, but I found that they were introducing regressions. It turned out that just simply making the
MultiSelectEpisodesHelpera singleton solved most of the multi-select issues I see.This almost fixes #98, but there is still a multiselect issue with bookmarks that I'm seeing. I'm not trying to address the one remaining multi-select issue in this PR just to keep this PR small and focused.
Testing Instructions
1. Watch for ANR regressions
I think it's very unlikely that this change would cause any ANR issues, but given that we had some ANR issues the last time we made significant changes to multiselect, please keep an eye out for any unexpected slowness as you test this change
2. Mult-Select Improvement
I tested this by turning on multiselect in the first screen, navigating to the second screen, and verifying that the second screen was not put into a multiselect state. When running through these tests, make sure you clear multiselect state from the relevant screens before testing (speaking from experience, it's easy to forget to do that and then to think a subsequent test is failing when it really isn't).
Let me know if there are any other test scenarios we should be covering here.
2.1. For each test:
2.2. Before these changes
2.3. Expected Results After These Changes
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml