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

Make MultSelectBookmarksHelper not a singleton #1580

Merged
merged 11 commits into from
Dec 13, 2023

Conversation

mchowning
Copy link
Contributor

Description

This is a follow-up to #1579. That PR made the MultiSelectEpisodesHelper a singleton, and this PR makes the other multi-select helper (MultiSelectBookmarksHelper) a singleton.

We can't just make the MultiSelectBookmarksHelper a singleton without any other changes like we could with the MultiSelectEpisodeHelper because we need to make sure that the PlayerContainerFragment, the BookmarksFragment it contains, and the BookmarksViewModel for those views are all using the same MultiSelectBookmarksHelper. The most straightforward way to accomplish this seemed to be to just expose the helper from the view model. I'm definitely open to other ideas for how to handle this though.

This builds on #1579 , and together they fix #98

Testing Instructions

1. Watch for ANR regressions

I think it's 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:

  1. Make sure both the start and end screens are not in multi-select mode
  2. Enter multiselection on the start screen
  3. Navigate to the end screen
  4. Verify that the end screen is not in a multi-select state

2.2. With changes in #1579, but without changes from this PR

  1. 🔴 Podcast Bookmarks → Full Screen Player Bookmarks
  2. 🟢 Full Screen Player Bookmarks → Podcast Bookmarks
  3. 🟢 Podcast Screen → Up Next
  4. 🟢 Up Next → Podcast Screen
  5. 🟢 Downloads/Starred/Listening-History → Up Next
  6. 🟢 Up Next → Downloads/Starred/Listening-History
  7. 🟢 Files → Up Next
  8. 🟢 Up Next → Files
  9. 🟢 Downloads/Starred/Listening-History → Podcast
  10. 🟢 Podcast → Downloads/Starred/Listening-History
  11. 🟢 Files → Podcast
  12. 🟢 Podcast → Files
  13. 🟢 Viewing a podcast on Discover tab → Viewing a podcast on Podcasts tab

2.3. Expected results With changes from this PR

  1. 🟢 Podcast Bookmarks → Full Screen Player Bookmarks
  2. 🟢 Full Screen Player Bookmarks → Podcast Bookmarks
  3. 🟢 Podcast Screen → Up Next
  4. 🟢 Up Next → Podcast Screen
  5. 🟢 Downloads/Starred/Listening-History → Up Next
  6. 🟢 Up Next → Downloads/Starred/Listening-History
  7. 🟢 Files → Up Next
  8. 🟢 Up Next → Files
  9. 🟢 Downloads/Starred/Listening-History → Podcast
  10. 🟢 Podcast → Downloads/Starred/Listening-History
  11. 🟢 Files → Podcast
  12. 🟢 Podcast → Files
  13. 🟢 Viewing a podcast on Discover tab → Viewing a podcast on Podcasts tab

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@mchowning mchowning added the [Type] Bug Not functioning as intended. label Dec 1, 2023
@mchowning mchowning added this to the 7.54 milestone Dec 1, 2023
@mchowning mchowning marked this pull request as ready for review December 1, 2023 03:11
@mchowning mchowning requested a review from a team as a code owner December 1, 2023 03:11
@mchowning mchowning modified the milestones: 7.54, 7.55 Dec 11, 2023
@ashiagr ashiagr force-pushed the fix/multiselect-shared-state_bookmarks branch from 84bf9a6 to 9c96ef3 Compare December 12, 2023 10:59
@ashiagr
Copy link
Contributor

ashiagr commented Dec 12, 2023

Looks good overall. I think we need these changes as well:

  • Podcast screen: Access multi-select helpers (for both episodes and bookmarks) in the PodcastFragment from the PodcastViewModel to
    • allow select all for Episodes and Bookmarks
    • allow editing bookmark
  • Player container screen: Share the view model scope between the parent container and the bookmarks fragment to
    • allow showing the (blue) multi-select toolbar on the Player - Bookmarks tab
  • Episode container and bookmarks (for files) container screens: Access multi-select helpers from the bookmarks view model in other containers like EpisodeContainerFragment and BookmarksContainerFragment

I tried these updates here. If you find them good, we can merge them here or I can create another PR.

mchowning and others added 4 commits December 12, 2023 10:30
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 mchowning force-pushed the fix/multiselect-shared-state branch from 3694267 to 11a22f2 Compare December 12, 2023 15:32
@mchowning mchowning force-pushed the fix/multiselect-shared-state_bookmarks branch from 9c96ef3 to c4a7c3b Compare December 12, 2023 17:15
@mchowning
Copy link
Contributor Author

mchowning commented Dec 12, 2023

I tried these updates here. If you find them good, we can merge them here or I can create another PR.

Changes look good to me. Thanks! I've rebased this PR onto the new code for #1579, which now targets release/7.54 (so this is now targeting release/7.54 not main). I then cherry-picked your changes onto this, and the only changes I made in addition to your changes were:

  1. Fixing a couple of references that needed to be updated (c4a7c3b)
  2. Updating the changelog to show this is going into 7.54 (2c3da17)

I also went ahead and ran through all the test scenarios again and didn't observe any issues.

Base automatically changed from fix/multiselect-shared-state to release/7.54 December 13, 2023 04:40
# Conflicts:
#	CHANGELOG.md
#	modules/features/player/src/main/java/au/com/shiftyjelly/pocketcasts/player/view/bookmark/BookmarksContainerFragment.kt
#	modules/features/podcasts/src/main/java/au/com/shiftyjelly/pocketcasts/podcasts/view/episode/EpisodeContainerFragment.kt
@ashiagr ashiagr enabled auto-merge (squash) December 13, 2023 04:58
@ashiagr ashiagr merged commit d24a1a8 into release/7.54 Dec 13, 2023
6 of 10 checks passed
@ashiagr ashiagr deleted the fix/multiselect-shared-state_bookmarks branch December 13, 2023 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Not functioning as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants