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

✨ Store listens when offline or if submission failed #521

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

Maxr1998
Copy link
Collaborator

@Maxr1998 Maxr1998 commented Nov 1, 2023

Useful if you want to manually export them to ListenBrainz or similar later on.

The biggest problem is that the exported file is not easily viewable at the moment. On newer Android versions, you can only view the files when attached to a computer (or with root permissions). On iOS, it's not possible at all.
However, we can probably build some export feature later on.

@Maxr1998 Maxr1998 force-pushed the offline-listen-log branch 2 times, most recently from 649ff10 to 9771317 Compare November 1, 2023 16:11
Useful if you want to manually export them to ListenBrainz or similar later on.
@Chaphasilor
Copy link
Collaborator

This is awesome! 😁

Could we write it to a Hive/Isar database instead and then export to file or share-sheet on-demand? 🤔
This way we could also easily use the data once Playback Reporting supports retroactively adding listens :D

@Chaphasilor
Copy link
Collaborator

Also, while there is a timestamp, there is neither a duration, nor an end timestamp, nor an event type (playback started/stopped). So I'm not sure if this would be sufficient to use as playback history data?

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Nov 1, 2023

Could we write it to a Hive/Isar database instead and then export to file or share-sheet on-demand? 🤔
This way we could also easily use the data once Playback Reporting supports retroactively adding listens :D

That would definitely be the more elaborate solution. I didn't want to bother with writing UI and database and export logic, but I suppose it would be sensible in the future.

Also, while there is a timestamp, there is neither a duration, nor an end timestamp, nor an event type (playback started/stopped). So I'm not sure if this would be sufficient to use as playback history data?

For my use case, even just the id/mbid and timestamp would be enough. With those, I'd map the data to a ListenBrainz listen history (maybe with the help of the ListenBrainz server plugin) and submit it.
Currently, only stop events, thus "full" listens are saved.

@jmshrv
Copy link
Owner

jmshrv commented Nov 2, 2023

ooo nice, this could be the start of actual playback reporting :)

Saving to a file directly is a bit finnicky, if we just want to export JSON we should "share" the file, similar to how exporting logs works

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Nov 2, 2023

Yeah, that'd be the best solution. I think for now, we could keep the PR as is, and I can look into a Hive db later on.

@Chaphasilor
Copy link
Collaborator

I guess we can get this merged. One thing I would like to have clarified, is the timestamp attribute the start time or the stop time? Maybe you could rename it to make it clearer...
Oh and adding the user ID wouldn't hurt either, just to make it a bit more compatible with the format of the Playback Reporting plugin :)

@Chaphasilor Chaphasilor self-assigned this Dec 5, 2023
@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 5, 2023

timestamp is the stop time, because that's what's usually tracked by Last.fm or ListenBrainz and the like. Let me know if I should add a documentation comment to it!

I'll just do that when adding the user ID 😄

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

Done, added the user ID, added a comment to the functions and renamed the item ID to clearly separate it from the user ID. Also tested this locally and confirmed everything still works.

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

By the way, feel free to squash this PR when merging. I think it can be considered a single path.

@Maxr1998 Maxr1998 changed the title ✨ Write offline listens to a file ✨ Store listens when offline or if submission failed Dec 6, 2023
@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

Went ahead and also made the code store listens in a Hive box. That should make adding a UI later on much easier.

@Chaphasilor
Copy link
Collaborator

Awesome, thanks for putting in the effort! I'll merge it later.

Any idea how many bytes per offline listen are added to the file? I'm wondering if it's necessary to clear the file at some point, put it's probably less than 1 kB per entry, so that shouldn't become a problem unless someone listens for hundreds of thousands of songs.
And once some UI is in place to export the file on-demand, we can just rely on the Hive box and get rid of the persistent file...

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

Any idea how many bytes per offline listen are added to the file? I'm wondering if it's necessary to clear the file at some point, put it's probably less than 1 kB per entry, so that shouldn't become a problem unless someone listens for hundreds of thousands of songs.

It depends on the length of the track titles and such, but in my checks it was 200-300 bytes per listen. So, you'd need 5k listens to reach just one Megabyte..

And once some UI is in place to export the file on-demand, we can just rely on the Hive box and get rid of the persistent file...

And yes, once the export logic is in place, we can simply delete the file.
We should however regularly clear the Hive box eventually. That should be really easy to implement however.

@Chaphasilor
Copy link
Collaborator

Awesome. Yeah we could truncate listens older than ~1.5 years on startup, or do we need to do it sooner? I don't have much experience with Hive yet...

If someone listens only in offline mode, I'd like to still have at least a year of playback data available for things like Jellyfin Rewind, at least until we can retroactively push listens to the Jellyfin server.

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

1.5 years sounds good to me. I listen to a lot of music and will reach 10k listens for the first time this year, and even people listening to 10x that should be fine with the <50 MB used.
Comparing it to CD quality FLAC, that's just two songs..

@Chaphasilor
Copy link
Collaborator

Right. Currently sitting at 56k listens for this year (plus max. 5k offline), and the most I ever had on Spotify was 95k. So I doubt anyone will surpass 150k in a year, which would be around 75 MB.
As long as Hive doesn't hiccup when opening a ~75MB box, we should be fine.

The comparion with the FLAC files probably doesn't really apply since the files are not app data and only loaded on-demand. But from a storage space used-perspective you're absolutely right.

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

As long as Hive doesn't hiccup when opening a ~75MB box, we should be fine.

I admit my focus was on storage space earlier, so I didn't think about how Hive would handle that data. I expected it to only load contents it needs and on demand, but it looks like this isn't the case. According to this hive ticket, we should probably look into SQLite instead..

@Chaphasilor
Copy link
Collaborator

isar/isar#1414

We could just migrate to Isar for that, we should do that at some point anyway :D

@Chaphasilor Chaphasilor merged commit 3ea4677 into jmshrv:main Dec 6, 2023
2 checks passed
@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Dec 6, 2023

Finally merged, thanks again for the PR!
I spent a few hours testing it and both regular playback reporting and offline reporting to file worked just fine :)

Edit: Forgot to squash, damn it 🙃
The code and commits in the repo are a mess anyway 💀

@Maxr1998
Copy link
Collaborator Author

Maxr1998 commented Dec 6, 2023

isar/isar#1414

We could just migrate to Isar for that, we should do that at some point anyway :D

I was looking at drift earlier, but Isar also looks really nice!

Finally merged, thanks again for the PR!
I spent a few hours testing it and both regular playback reporting and offline reporting to file worked just fine :)

Thank you so much! Happy to have this feature merged now.

Edit: Forgot to squash, damn it 🙃
The code and commits in the repo are a mess anyway 💀

lol rip. It's fine, but I suppose you could disable merge commits and only allow rebase or squash merges in the future to keep the repo cleaner.

@Maxr1998 Maxr1998 deleted the offline-listen-log branch December 6, 2023 23:25
@Chaphasilor
Copy link
Collaborator

@Maxr1998 I just ported this feature to the redesign, maybe you could take a quick look to confirm I got everything right (8e5e50c)

I also added a share button to the Playback History screen (in 22a5b93), which should make it much easier to access the offline listens log file :)

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