sindresorhus / wallpaper

Manage the desktop wallpaper
MIT License
1.05k stars 92 forks source link

Promisify Linux version #8

Closed sindresorhus closed 8 years ago

sindresorhus commented 9 years ago

This file: https://github.com/sindresorhus/wallpaper/blob/4d1dbc3e582328cb1e0da65b94ebe6191f73bbe5/lib/linux.js

I just wrapped it in pify for now: https://github.com/sindresorhus/wallpaper/blob/a1d20f1703135a1a8f3e302eae234ce4dc6bb002/index.js#L8

// @SamVerschueren @kevva @arthurvr in case any of you are interested. No worries if not. If you're doing it, comment so we don't end up with duplicated work.

SamVerschueren commented 9 years ago

On it!

SamVerschueren commented 9 years ago

@sindresorhus Can you explain more in detail what this does?

    availableApps.forEach(app => {
        if (!app.set) {
            return;
        }

        const params = JSON.parse(JSON.stringify(app.set));
        params[params.length - 1] = params[params.length - 1].replace('%s', imagePath);

        childProcess.execFile(app.cmd, params, err => cb(err));
    });

It calls the callback for every availableApps entry, is this correct? Doesn't seem like it should call the callback like 10 times.

sindresorhus commented 9 years ago

It goes through all the found "apps" and executes its set command. In hinsight this is very wrong, and it should only execute and callback the first found that has a set method, not all of them.

SamVerschueren commented 9 years ago

Thanks for the clarification, will fix it.