Wrong album art when title includes ampersand (no urlencode on GET request ?)

[scaprile]

Hi there,
using latest version from play store, album art off the Internet.
Albums like Dream Theater's Black Clouds & Silver Lining or Maceo Parker's Roots & Grooves get wrong album art. This looks as if an HTTP GET request is not urlencoded and so the '&' is interpreted by the server as a field separator.
Best regards,

[hurzl]

Also the case for local covers no, no, other problem

[hurzl]

Yes it's true... If the album name contains "&", the local cover search takes the 1st track filename as a directory and searches in subdirs of it. Instead of using the parent directory.
And it's not the path name, it doesn't contain "&".

[scaprile]

In MPDroid/src/main/java/com/namelessdev/mpdroid/cover/AbstractWebCover.java, instead of properly urlencoding the text, I see this attempt to change spaces to %20. This will easily break with any non-english language, and probably also with hyphens in album names (although I didn't check):

protected String executeGetRequest(final String rawRequest) {
    final HttpGet httpGet;
    final String httpRequest;
    final String response;

    httpRequest = rawRequest.replace(" ", "%20");

In my limited knowledge, cause I do embedded C, I understand that the proper way to do this is to use the URI class and its toURL() method. At top level you build a hierarchical URI with the query and then construct a proper URL before sending the GET request.
http://docs.oracle.com/javase/6/docs/api/java/net/URI.html

However, while browsing the source code, this will probably involve a lot of rewriting, so a package like URLEncoder would be a better option.
Basically, an ampersand in either the artist or the album name will break the GET request, as can be seen on (for example) MPDroid/src/main/java/com/namelessdev/mpdroid/cover/LastFMCover.java:

request = URL + "?method=album.getInfo&artist=" + albumInfo.getArtist() + "&album="
+ albumInfo.getAlbum() + "&api_key=" + sKey;

Both albumInfo.getArtist() and albumInfo.getAlbum() need to be urlencoded. A trivial solution like the above "space escaping" can't be applied here since it will break the '&' used for parameter separation.

Should I try to fix it myself ?

[abarisain]

We had a lot of problems with that code (and seriously rewritten it a THOUSAND times), especially with slashes ending up URL Encoded, which broke. Don't know why it's that way, but you also have to take into account that even with the builder, we have to use user added URLs that may contain slashes, that shouldn't be escaped. so it's really annoying to deal with.

[hurzl]

Can't the strings be escaped just when coming into this cover searching stuff? You would avoid confusion with parameter sep.

[hurzl]

And what does the local server problem have to do with it?

[abarisain]

Hmm they could be escaped before yeah.

What's the local server problem ?

[hurzl]

Using files as directories and searching for cover.jpg there

[scaprile]

Perhaps:

request = URL + "?method=album.getInfo&artist=" + URLEncoder.encode(albumInfo.getArtist(),"utf-8") + "&album="+ URLEncoder.encode(albumInfo.getAlbum(),"utf-8") + "&api_key=" + sKey;

will do ? (at least in LastFMCover.java)

[hurzl]

Oh I see, you pass AlbumInfo, so you will have to do this everywhere ...

[hurzl]

I would change the ICoverRetriever.getCoverUrl to use album and artist name Strings (have to rewrite all implementing classes), and URLencode these in CoverManager.
Or do you need more than album and artist for searching? -- Yes in LocalCover of course ...

[hurzl]

The local cover problem occurs when I press "reset cover", then the AlbumInfo.mParentDirectory suddenly contains the first track's filename

[scaprile]

After browsing some files, I still think it is easier to just urlencode inside specific superclasses for each provider; they build the query string and extend AbstractWebCover, all query strings are different.

[hurzl]

One could add a getCoverUrl(Albuminfo) in AbstractWebCover which calls
the getCoverUrl(String album, String artist) in its subclasses with the urlencoded strings

like this

    public abstract String[] getCoverUrl(String artist, String album) throws Exception;

    public final String[] getCoverUrl(AlbumInfo albumInfo) throws Exception {
        return getCoverUrl(URLEncoder.encode(albumInfo.getArtistName(),"utf-8"),
                    URLEncoder.encode(albumInfo.getAlbumName(),"utf-8"));
    }

[scaprile]

Oh, I see, my knowledge of object oriented and Java is very limited. LocalCover still requires AlbumInfo as a parameter cause it uses albumInfo.getFilename(), but I guess method overloading handles this ?
Yes, just saw your commit... Thanks

[hurzl]

Yes, LocalCover is not an AbstractWebCover so it's not changed here

[scaprile]

Any chances to have this merged/backported and updated in Play store in a reasonable future or should I get a zip and build it myself ? Thanks!

[abarisain]

We're really really late on play store releases anyway, and will be for a
long time, so I suggest you build hurzl's fork yourself. Sorry for the
hassle.

2015-04-27 20:33 GMT+02:00 scaprile notifications@github.com:

Any chances to have this merged/backported and updated in Play store in a
reasonable future or should I get a zip and build it myself ? Thanks!

—
Reply to this email directly or view it on GitHub
#714 (comment).

[hurzl]

My fork has also a lot of other patches, some of which are bugfixes, too, and I am waiting to be accepted for ages ...

[scaprile]

No problem at all, thank you both