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

Add PlayerRepository to media module #173

Merged
merged 3 commits into from
May 17, 2022
Merged

Conversation

luizgrp
Copy link
Member

@luizgrp luizgrp commented May 16, 2022

WHAT

Add PlayerRepository and domain models to media module.

WHY

In order to have a PlayerRepository that is independent of Media3 dependencies.

Checklist 📋

  • Add explicit visibility modifier and explicit return types for public declarations
  • Run spotless check
  • Run tests
  • Update metalava's signature text files

Copy link
Collaborator

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

Nothing blocking and probably easier to have the next batch of discussions on incremental PRs.

@luizgrp luizgrp force-pushed the media_playerrepo branch 2 times, most recently from 90ec861 to 7da3900 Compare May 16, 2022 15:57
Comment on lines 23 to 24
val title: String? = null,
val artist: String? = null

Choose a reason for hiding this comment

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

what is a media item that has neither title nor artist. Is this sth we want to support?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yschimke please see the above, I ported this way to follow the implementation in the media toolkit, was there any use case for that in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dpreussler what would be your suggestion?

a) non-nullable title?
b) non-nullable title and artist?
c) keep both them nullable but enforce clients to provide one or the other via factory methods?

please let me know if you have a different idea in mind

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we think in 95% of apps using this code we should avoid the hassle of nullability and require them. Maybe just a note that empty "", should be used if there legitimately isn't one.

There can be multiple artists (track artist, album artist) perhaps we should make this clear it's whatever we want for display? displayArtist. But I don't know, so happy to go with non nullable artist for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

made the changes re non nullable artist

good point re display artist, will land it as it is and we can evolve it from there as this require further discussion

@luizgrp luizgrp force-pushed the media_playerrepo branch from 7da3900 to a5c032f Compare May 17, 2022 13:17
@luizgrp luizgrp merged commit 8b0c084 into google:main May 17, 2022
@luizgrp luizgrp deleted the media_playerrepo branch May 17, 2022 13:55
This was referenced May 19, 2022
@luizgrp luizgrp self-assigned this Dec 8, 2022
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.

3 participants