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

MOBILE-160,184: Listen Scrobble fixes and improvements #336

Merged
merged 17 commits into from Jan 22, 2024

Conversation

07jasjeet
Copy link
Contributor

@07jasjeet 07jasjeet commented Jan 14, 2024

Closes MOBILE-160 and MOBILE-184.

Also Closes #331 and #324

Summary

Now onwards, whenever a new media app is discovered, it is opted out by default. Only user can opt-in the newly discovered media app. This helps avoid unintended scrobbles with newly discovered apps.
Migration from Blacklist to Whitelist has been handled.

Copy link
Member

@akshaaatt akshaaatt left a comment

Choose a reason for hiding this comment

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

Thanks for working on this PR but we should have discussed this before implementing. We simply cannot move to whitelisting. This changes the UX entirely and we were not in the favour of doing this from the beginning. We want to reduce the number of steps the user takes to start submitting listens. We need to have a discussion on this properly before moving ahead with this work.

@akshaaatt akshaaatt marked this pull request as draft January 15, 2024 05:28
@07jasjeet 07jasjeet marked this pull request as ready for review January 18, 2024 11:58
@07jasjeet 07jasjeet changed the title MOBILE-160: Listen Scrobble is now opt-in MOBILE-160,184: Listen Scrobble is now opt-in Jan 18, 2024
@07jasjeet 07jasjeet changed the title MOBILE-160,184: Listen Scrobble is now opt-in MOBILE-160,184: Listen Scrobble can now be opt-in or opt-out Jan 18, 2024
@07jasjeet 07jasjeet changed the title MOBILE-160,184: Listen Scrobble can now be opt-in or opt-out MOBILE-160,184: Listen Scrobble fixes and improvements Jan 18, 2024
@akshaaatt
Copy link
Member

@07jasjeet why is this PR made against main and not dev?

@07jasjeet 07jasjeet changed the base branch from main to dev January 20, 2024 18:40
@07jasjeet
Copy link
Contributor Author

@07jasjeet why is this PR made against main and not dev?

Oops, forgot to change it.

1) Renamed `ListenService` to `ListeningService` to avoid confusion with `ListensService`.
2) Fixed Dependency bump of work manager which caused a crash.
1) Removed unnecessary initialisation on ListenBrainzTheme in BrainzPlayerScreen
2) Fixed ListenCardSmall's dropdown ratio in certain case and added preview.
3) We do not need to recreate the screen now as everything depends upon ListenBrainzTheme.colorScheme now.
build.gradle Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
1) Revert gradle version change.
2) Changed name of ListenService and ListensDatabase.
Copy link
Member

@akshaaatt akshaaatt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! 💯

@akshaaatt akshaaatt merged commit 9ed403d into metabrainz:dev Jan 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

don't automatically scrobble any player
2 participants