added swaybg support #93
Conversation
|
Please fix the reported lint issues and also follow the style of the other platforms. |
|
Also I promisified 'spawn' |
| } | ||
|
|
||
| export async function set(imagePath) { | ||
| await spawn('swaybg', ['-i', imagePath, '-m', 'fill']); |
There was a problem hiding this comment.
Use long-flags if possible, for readbility.
| } | ||
|
|
||
| export async function set(imagePath) { | ||
| await spawn('swaybg', ['-i', imagePath, '-m', 'fill']); |
There was a problem hiding this comment.
Any why can't you use the existing execFile?
There was a problem hiding this comment.
Because swaybg must work constantly to set wallpaper. When I use execFile the serWallpaper function never completes due to which any code written after the function is not executed.
There was a problem hiding this comment.
Looks like you can use swaymsg.
https://github.com/search?q=swaymsg+swaybg&type=code
There was a problem hiding this comment.
For swaymsg you must install sway but you can use swaybg without sway
There was a problem hiding this comment.
Alright. https://github.com/sindresorhus/wallpaper/pull/93/files#diff-96da661640c7b1ea27ced9841f06a8dab68749a29c97cd37b39ead556530da9fR7 should be moved back into this file. It doesn't make sense as a generic utility.
| } | ||
|
|
||
| export async function set(imagePath) { | ||
| await spawn('swaybg', ['-i', imagePath, '-m', 'fill']); |
There was a problem hiding this comment.
Alright. https://github.com/sindresorhus/wallpaper/pull/93/files#diff-96da661640c7b1ea27ced9841f06a8dab68749a29c97cd37b39ead556530da9fR7 should be moved back into this file. It doesn't make sense as a generic utility.
source/linux/util.js
Outdated
| args, | ||
| ) => new Promise(resolve => { | ||
| const cp = childProcess.spawn(cmd, args); | ||
| cp.on('spawn', () => { |
There was a problem hiding this comment.
This needs a code comment on why we resolve on spawn, which makes no sense without the context.
source/linux/util.js
Outdated
| if (code) { | ||
| console.log(`swaybg exited with ${code} code`); | ||
| } | ||
|
|
||
| if (signal) { | ||
| console.log(`swaybg exited with ${signal} signal`); | ||
| } |
There was a problem hiding this comment.
This should be properly handled with reject/resolve and not log.
There was a problem hiding this comment.
I thought about that then I realized that swaybg can't exit from invalid argument and the only way that user can stop swaybg(accidentally or not) is writing wrong flag or killing it by themself
There was a problem hiding this comment.
So I just removed that part of code
| await spawn('swaybg', ['--image', imagePath, '--mode', 'fill']); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } |
There was a problem hiding this comment.
Don't catch and log the error.
| cp.stderr.on('data', data => { | ||
| if (data.includes('Failed to load image')) { | ||
| cp.kill('SIGINT'); | ||
| reject(); |
There was a problem hiding this comment.
You need to preserve the error message. Promise rejecting without anything is very useless to an end-user using this library.
There was a problem hiding this comment.
I recovered the error message, but I don't understand why I need this error message if I didn't catch it.
Co-authored-by: Sindre Sorhus <[email protected]>
Added swaybg support