Switch mpris cover art cache to use a hash of the url

[ferrreo]

This fixes an issue where if the track + artist name is missing you would get whatever the cover art for the first track with those missing you listened to is. This way as long as there is a url for the cover art you will always get the correct one back.

The original bug happens for me when opening spotify with a track paused as it will return cover art via mpris but no artist or track title (gotta love spotify bugs).

[Aylur]

I feel like a cryptographic hash function is a but overkill for this, why not just use the url instead of artist+title?

[ferrreo]

Using a hash as a cache key is an extremely common usecase for hashes. It ensures a minimum of clashes whilst maximising hit rates and ensuring a consistent format that doesn't need extra processing for length/format to work with fillesystem etc.

I have added another improvement, a cache index limited to 100 that repopulates on restart and will purge the oldest entry if it goes over the 100. I added this because I noticed my media cache dir was hitting 3.5gb in size which is a bit excessive.

[Aylur]

I like the idea of deleting cache if it hits a certain point
There should be a CacheService, and then Mpris, Notifications, Applications could use it and so could the user for custom stuff
then we could remove the clear-cache cli option since it would be obsolete

[Aylur]

it should be

if (!this.trackCoverUrl)
  return

and mark coverPath as optional

[ferrreo]

If we don't set this.coverPath here if we go from song with cover to song without cover it will continue to have the old cover showing instead of a blank/no cover.

[Aylur]

this is an async function this return value wont get assigned to coverPath
and even if there was an error caching, the function continues to mark it cached in the json file

[ferrreo]

done

[Aylur]

instead of hardcoding it to 100, there should be an option in App.config for it

[ferrreo]

done

[Aylur]

use Utils.writeFile instead

[ferrreo]

done

[ferrreo]

I like the idea of deleting cache if it hits a certain point There should be a CacheService, and then Mpris, Notifications, Applications could use it and so could the user for custom stuff then we could remove the clear-cache cli option since it would be obsolete

Will work on making this a service tonight

[ferrreo]

I like the idea of deleting cache if it hits a certain point There should be a CacheService, and then Mpris, Notifications, Applications could use it and so could the user for custom stuff then we could remove the clear-cache cli option since it would be obsolete

Will work on making this a service tonight

done

[Aylur]

for some reason this breaks mpris for me, ListNamesRemote runs twice on startup once correcty and again with the names undefined, but even though the names are correctly checked they are not added
seems like it hangs on the new MprisPlayer(busName) line

[Aylur]

I was thinking something like this

class CacheObject {
    static {
        Service.register(this, {
            'added': ['string'], // signal the path or url
            'removed': ['string'], // signal the path or url
        });
    }

    private _limit: number;
    private _path: string;
    private _cache: {
        [sha: string]: {
            path: string,
            timestamp: number,
        }
    };

    // or consider using a Map object for performance
    // although this needs a bit more work to write and read from a json file
    private _cacheMap: Map<string, {
        path: string,
        timestamp: number,
    }>;

    // url is the input for the sha, returns the path to file
    getImage(url: string): string { }

    // cache, or overwrite it and check for the oldest
    addImage(url: string): Promise<string>;
}

class CacheService {
    // no signals for this

    private _caches: { [name: string]: CacheObject };

    constructor() {
        // reload from caches.json that stores the paths to the cache json files
    }

    // creates a new CacheObject or returns the existing one
    getCache(name: string): CacheObject
}

the cache user can then decide whether to overwrite the image by calling addImage or not

I would also consider updating the .json files only when the app has quit, although this might not matter that much

App.instance.connect('shutdown', writeFile)

[ferrreo]

I like the simplified design I'll update to that tomorrow and fix the conflicts.

I don't think writing on shutdown is a good idea or you risk orphans on an improper shutdown and it isn't exactly costly. Though a batch add might be useful so you can cache many things at once and only write the index once.

As for the issue in startup, I've not changed the flow around that at all? Maybe the changes have uncovered/made worse an existing race condition? It sounds similar to the issues I also get on the original mpris code. I'll try replicate and fix.

[Eclextic]

I like the simplified design I'll update to that tomorrow and fix the conflicts.

Well that aged like fine wine XD