Skip to content
This repository was archived by the owner on Feb 4, 2023. It is now read-only.

Add check whether song still exists#403

Open
doompadee wants to merge 1 commit intokarimknaebel:masterfrom
doompadee:validate-songs
Open

Add check whether song still exists#403
doompadee wants to merge 1 commit intokarimknaebel:masterfrom
doompadee:validate-songs

Conversation

@doompadee
Copy link
Copy Markdown
Contributor

Validate songs upon loading

Fixes #396

Signed-off-by: doompadee <31240866+doompadee@users.noreply.github.com>
@karimknaebel
Copy link
Copy Markdown
Owner

Not sure if I should merge this. Have you already tested performance? 1000+ IO operations don't seem like a good idea.

@karimknaebel
Copy link
Copy Markdown
Owner

In my opinion either we trust the media store, or we check everything ourselves.

@karimknaebel
Copy link
Copy Markdown
Owner

We should implement some sort of check only when the data is loaded from the saved queue database. This is another problem, because there is nothing that syncs changes to the playing queue or checks its integrity.

@doompadee
Copy link
Copy Markdown
Contributor Author

I did not think about performance implications at all! For lists with 1000s of songs, there would probably be some noticeable overhead. Personally, I don't use the Songs view and don't have huge playlists...

But instead of entirely neglecting the issue, I think we could find some middle ground right away. The media index does not seem too trustworthy to me.

So what about a threshold that only performs the file system check if the cursor only contains up to a certain of number rows? This would at least cover some cases and in my case, all relevant cases. Not ideal, but better than nothing. Checking 30 files probably won't have an impact on perceived performance?

@karimknaebel
Copy link
Copy Markdown
Owner

If we don't trust the media store here, we can't trust the media store on metadata etc. too. Also in this specific case the issue doesn't have to do something with the media store. The playing queue is saved to a database and we don't update this database when a song is deleted. It's our own fault actually. I don't see a simple way to fix this unfortunately.

@doompadee
Copy link
Copy Markdown
Contributor Author

Oh, I was not aware that a custom database is involved here. That explains why the file deletion goes unnoticed.

I will adjust to only check the playing queue. Or maybe look into house keeping like adding a ContentObserver or something.

@karimknaebel
Copy link
Copy Markdown
Owner

It's done via this class: https://github.com/kabouzeid/Phonograph/blob/master/app/src/main/java/com/kabouzeid/gramophone/provider/MusicPlaybackQueueStore.java

When the media store changed, the musicservice must sync the playing queue with the media store.

I don't see a simple solution here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants