Add database #443
Add database #443
Conversation
| suspend fun getCount(): Int | ||
|
|
||
| @Insert(onConflict = OnConflictStrategy.REPLACE) | ||
| suspend fun insert( |
There was a problem hiding this comment.
not sure if there is a more straight forward approach with room where a single entity/pojo could be passed
| @OptIn(FlowPreview::class) | ||
| override fun get(playlistId: String): Flow<PlaylistDownload> = | ||
| playlistLocalDataSource.getPopulated(playlistId).flatMapMerge { populatedPlaylist -> | ||
| combine( |
There was a problem hiding this comment.
this flatMapMerge + combine could potentially be removed if figuring out a way for the DAOs to return a pojo with the combined data from PlaylistEntity + MediaEntity + MediaDownloadEntity
There was a problem hiding this comment.
+1 We can follow up, but be nice to start using this, so maybe follow up?
| override fun getPlaylists(): Flow<Result<List<Playlist>>> = playlistCache | ||
| override fun getAllPopulated(): Flow<List<Playlist>> = flow { | ||
| emitAll( | ||
| if (playlistLocalDataSource.isEmpty()) { |
There was a problem hiding this comment.
temporary approach for offline first, to be improved
...a/com/google/android/horologist/mediasample/data/service/download/DownloadManagerListener.kt
Show resolved
Hide resolved
...-sample/src/main/java/com/google/android/horologist/mediasample/domain/PlaylistRepository.kt
Show resolved
Hide resolved
| import androidx.room.PrimaryKey | ||
|
|
||
| @Entity | ||
| data class PlaylistEntity( |
There was a problem hiding this comment.
Before this gets pulled up from samples, we should discuss this name. Playlist might be quite specific compare to album, so maybe there is a more general name like collection, with a type field?
...ple/src/main/java/com/google/android/horologist/mediasample/data/database/dao/PlaylistDao.kt
Show resolved
Hide resolved
.../java/com/google/android/horologist/mediasample/ui/playlists/UampPlaylistsScreenViewModel.kt
Show resolved
Hide resolved
| @ColumnInfo val mediaUrl: String, | ||
| @ColumnInfo val artworkUrl: String, | ||
| @ColumnInfo val title: String?, | ||
| @ColumnInfo val artist: String?, |
There was a problem hiding this comment.
For later: Will we add some mechanism to have additional fields, say in a child table? We may not need them and the cost of loading/parsing for most screens. But we can assume that each app has additional fields they will want to show. Think a podcast?
There was a problem hiding this comment.
good idea, will address when moving to the lib 👍
| ) | ||
| suspend fun getCount(): Int | ||
|
|
||
| @Insert(onConflict = OnConflictStrategy.REPLACE) |
There was a problem hiding this comment.
Is REPLACE here for the mediaEntity list?
For the others, they shouldn't already exist, or if they do we are building up a bigger set over time, as we are not removing existing entries?
I wonder if it's better to define this as three operations like the first sample here https://developer.android.com/reference/androidx/room/Transaction
There was a problem hiding this comment.
yeah I thought about this too, will improve this once implementing the database sync mechanism 👍
There was a problem hiding this comment.
Just checked the implementation of this function in PlaylistDao_Impl.java and it looks like it uses a transaction:
__db.beginTransaction();
try {
__insertionAdapterOfPlaylistEntity.insert(playlistEntity);
__insertionAdapterOfMediaEntity.insert(mediaEntityList);
__insertionAdapterOfPlaylistMediaEntity.insert(playlistMediaEntityList);
__db.setTransactionSuccessful();
return Unit.INSTANCE;
} finally {
__db.endTransaction();
}still can't use the function as it is as we need different strategy per table but just a FYI/TIL
| ) | ||
| data class PlaylistDownloadEntity( | ||
| data class PlaylistMediaEntity( |
There was a problem hiding this comment.
For later: Should playlists or albums have some sort of ordering concept?
There was a problem hiding this comment.
yeah I see there is a ordering field coming from the API, but it's per album and not per "playlist" so using that would conflict as we have more than one album per playlist, something to implement separately and discuss the strategy to take, but I agree to add this feature 👍
| @OptIn(FlowPreview::class) | ||
| override fun get(playlistId: String): Flow<PlaylistDownload> = | ||
| playlistLocalDataSource.getPopulated(playlistId).flatMapMerge { populatedPlaylist -> | ||
| combine( |
There was a problem hiding this comment.
+1 We can follow up, but be nice to start using this, so maybe follow up?
|
Landing and will follow up on comments |
WHAT
WHY
In order to allow implementing offline first strategy.
HOW
Checklist 📋