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

Null artist on search #267

Closed
DanielSevillano opened this issue Dec 23, 2023 · 11 comments
Closed

Null artist on search #267

DanielSevillano opened this issue Dec 23, 2023 · 11 comments
Labels
bug Something isn't working Fixed

Comments

@DanielSevillano
Copy link

Steps to reproduce the bug

Open the app and search.

Expected behavior

The results should show the artist and the duration of the songs.

Actual behavior

The results have null artists and they show the view count instead of the duration.

Screenshots/Screen recordings

Logs

No response

RiMusic version

0.6.14

Android version

Android 13

Additional information

No response

@DanielSevillano DanielSevillano added the bug Something isn't working label Dec 23, 2023
@fast4x
Copy link
Owner

fast4x commented Dec 24, 2023

Hello, it is not a bug, i change it in enhancement because the information about the artist does not come in the search for songs, so it must be implemented.

@fast4x fast4x added enhancement New feature or request New Idea New idea ti be explored and removed bug Something isn't working labels Dec 24, 2023
@DeadSecTr
Copy link

Can we leave it just "empty"

@fast4x
Copy link
Owner

fast4x commented Dec 26, 2023

Yep, is just empty

@locxter
Copy link

locxter commented Dec 30, 2023

I'm afraid to disagree, but this really is a bug. I have been using the same build of ViMusic for well over a year now (since it hasn't been updated on F-Droid since) and until just recently the search would always show the artist and play time for songs available on YouTube Music. Now it just shows the reduced information available for videos - this is not normal.

YouTube probably changed their API for songs a bit and now the reverse-engineered InnerTube client falls back to its video parsing methods, which results in this misbehaviour. I'm going to look into this issue a bit myself, but, since I haven't worked with Android applications before and certainly none so complex, help from some of the more experienced developers out here would be amazing!

Thanks in advance!

PS: Also mentioned this issue in the original ViMusic repo here.

@fast4x
Copy link
Owner

fast4x commented Dec 30, 2023

That’s right, probably YouTube changed the API, so it’s not a bug but a new feature because the structure of the API call needs to be changed. I will also work on this, at the moment it does not create problems when running the application.

@locxter
Copy link

locxter commented Dec 30, 2023

If that's your definition of a bug, I have to totally agree from a developer standpoint. However, from a user standpoint the app doesn't quite do what it is supposed to do and that feels like a bug. But enough of this definition back and forth - I'm really glad that you're also looking into it :)

@locxter
Copy link

locxter commented Dec 30, 2023

Okay, I have found the API change with this simplespython script to replicate the internal song search for "Like Woo" with an expected artist of "Kontra K" and duration of "2:28":

from innertube import InnerTube
import json

client = InnerTube(client_name="WEB_REMIX", client_version="1.20220606.01.00", user_agent="Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.157 Safari/537.36", api_key="AIzaSyC9XL3ZjWddXya6X74dJoCTL-WEYFDNX30")

data = client.search("Like Woo")
pretty = json.dumps(data, indent=4)

f = open("output.json", "w")
f.write(str(pretty))
f.close()

Apparently, Google added another FlexColumn to all songs featuring their play count, which moved the relevant FlexColumn with the artist name, play time and so on from the last one to the middle (index 1). Take a look at lines 730 to 810 in the attached JSON file for more context.

output.json

A fix should be as easy as changing line 26 in MusicShelfRenderer.kt from ?.lastOrNull() to ?.getOrNull(1). I will create a pull request shortly...

@fast4x
Copy link
Owner

fast4x commented Dec 30, 2023

Wow! Great, great job! Thank you for your time.

fast4x added a commit that referenced this issue Dec 30, 2023
Fixed issue #267 "Null artist on search".
@gregzerba
Copy link

gregzerba commented Dec 30, 2023

This will likely be part of a fix for an issue I was investigating where playlists were crashing when they were opened. This was because of the displaying of the full duration of a playlist. The durationToMillis() function assumed the duration string was in a time format but crashed when it showed the number of plays. The string was incorrect in the same way as the artist string because of the API change. I hadn't made an issue for it yet because I was wtill looking into it but it seems you saw this as well based on this commit: ddf8ba4

Now that the strings are correct again, the durationToMillis() function should work. However, database entries that were added between the API change and the release of this change will still have the incorrect duration string. A short-term fix might be adding exception handling to durationToMillis() and just use 0ms if it's not in the correct format.

Long-term I see 2 ways that this could be addressed.

  1. Whenever using the durationText fields in the Song table of the sqlite database and the duration text is in an improper format, request the information from the InnerTube API first to repair the database record and then continue.
  2. Personally, I feel the duration text and artist text don't really belong in the Song database record. The artists can be derived from the SongArtistMap table and the duration text should be stored in milliseconds. Storing this information here is redundant. When an artists string is needed, it can be generated on the fly with a JOIN operation on the SongArtistMap table on-the-fly. Similarly, when the duration text is needed, the milliseconds can be converted into a string representation on-the-fly.

One benefit to not storing strings like this that could be derived on-the-fly is that changes in the InnerTube API like this won't result in broken databases that now contain duration strings that don't contain text in the correct format. This would improve long-term stability even when youtube changes their API without breaking existing databases.

I realize that this database schema was inherited from ViMusic (and could be mirroring the result format of innertube? I'm not familiar with the API). Purely from looking at the database schema, I believe it could be optimized but I realize it may not be worth it based on legacy compatibility and effort to implement.

Personally, from a design perspective, I feel that the current schema isn't ideal and it would be nice to update, however, updating the schema would end up taking a lot of work throughout the code so it could very likely be not not worth it. In addition to the fact that people may want to be able to backup/restore between ViMusic and RiMusic and this would break compatibility. Just wanted to toss out the possibility based on what I saw.

@fast4x
Copy link
Owner

fast4x commented Dec 31, 2023

Hello, I understood that the problem was the function and I commented momentarily in order to solve the crash.
For the database I did not consider adding changes to the original structure of ViMusic. I do not exclude doing so in the future.

In the meantime I will manage the exceptions in the function, in order to insert the listening time of a playlist.

@fast4x fast4x added bug Something isn't working Fixed and removed New Idea New idea ti be explored enhancement New feature or request labels Dec 31, 2023
@fast4x
Copy link
Owner

fast4x commented Jan 5, 2024

Available from 0.6.16 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Fixed
Projects
None yet
Development

No branches or pull requests

5 participants