Add support for nitrogen on linux #17

fa7ad · 2016-09-01T22:00:52Z

This is popular among people who use tiling window managers (specially i3 users)

sindresorhus · 2016-09-02T07:05:49Z

Is there any way to get the wallpaper with nitrogen?

fa7ad · 2016-09-02T20:08:18Z

Not that I know of, besides reading the config file.

A simple way to do that would be,

grep -m 1 -oP '(?<=file=).*' ~/.config/nitrogen/bg-saved.cfg

Or, if you want more portability,

sed -n 's/^file=//p' ~/.config/nitrogen/bg-saved.cfg | tail -n 1

^ the tail bit and -m 1 is there to make sure it returns only one file name. Nitrogen can be launched from different environments and the configs are stored separately. But for most use cases, the last match is the last used settings and is likely what the user is looking for. but, I think these transformations are better handled in JS. no idea how to implement it though.

fa7ad · 2016-09-06T03:10:31Z

Do you have any suggestions on how a different command can be used to get values? With the current API, that doesn't seem to be possible.

sindresorhus · 2016-09-06T13:09:12Z

You could add manual workaround here

wallpaper/lib/linux.js

Line 126 in 72f0843

if app is nitrogen.

sindresorhus · 2016-10-22T21:39:00Z

ping :)

fa7ad · 2016-10-22T21:40:05Z

totally forgot about it, thanks for the ping. gonna make the commit asap 😄

Temporary workaround for #18

fa7ad · 2016-10-23T12:33:44Z

I think now you can merge it.

sindresorhus · 2016-11-04T10:00:48Z

lib/linux.js

@@ -32,6 +44,12 @@ const appsList = [
 		set: ['--bg-scale', '%s']
 	},
 	{
+		cmd: 'nitrogen',
+		set: ['--set-zoom-fill', '--save', '%s'],
+		get: ['-n', 's/^file=//p', `${process.env.HOME}/.config/nitrogen/bg-saved.cfg`],


Is there a long flag for -n? If yes, use that instead.

Use require('os').homedir() instead of the environment variable. Should also use path.join.

sindresorhus · 2016-11-04T10:02:22Z

lib/linux.js

@@ -124,6 +130,10 @@ exports.get = function get() {

 	const app = availableApps.find(app => app.get);

+	if (/nitrogen/.test(app.cmd)) {


if (app.cmd === 'nitrogen') {

sindresorhus · 2016-11-04T10:03:07Z

Can you look into the feedback from @Sohail05?

fa7ad · 2016-11-04T10:51:58Z

@sindresorhus I will fix those as soon as I get some time.
FWIW, I think this solution is very fragile. I'll try to re-iterate and maybe use node's fs module for get instead of relying on sed.

sindresorhus · 2016-11-04T10:53:05Z

I'll try to re-iterate and maybe use node's fs module for get instead of relying on sed.

👍

sindresorhus · 2016-11-29T08:15:35Z

lib/linux.js

@@ -159,5 +173,13 @@ exports.set = function set(imagePath) {

 	params[params.length - 1] = params[params.length - 1].replace('%s', path.resolve(imagePath));

+	if (app.cmd === 'gsettings') {
+		isCinnamon().then(cinnamon => {


This won't work. This check is async, but you're assigning to the outside scope which will be done by the time this is executed.

what do you suggest?

Changing it so it doesn't assign to the outside scope.

so checking it synchronously shouldn't be a problem right?

The exported method is async, so it shouldn't contain any synchronous calls.

I'm out of ideas on how to do it properly 😟

It's just promises. You can do anything you would do synchronously. You just need to chain the isCinnamon() before cp.execFile and return the whole chain.

sindresorhus · 2016-11-29T08:16:55Z

lib/linux.js

@@ -123,15 +113,39 @@ function setAvailableApps() {
 	});
 }

+function isCinnamon() {
+	const args = ['writable', 'org.cinnamon.desktop.background', 'picture-uri'];
+	// output = childProcess.execFile('gsettings', args, ());


just to maintain line length :P or are you talking about the comment?

The comment. Don't leave commented out code in there. That's what git is for.

yeah sorry about that, i thought i deleted the comments

sindresorhus · 2016-11-29T08:17:08Z

lib/linux.js

+	return cp.execFile('gsettings', args)
+		.then(out => (out.toString().trim() === 'true'))
+		.catch(err => {
+			err.toString(); // do nothing, this error is expected


This should simply be:

.catch(() => {});

i cant reproduce rn, but i think your linter complains about that if there's nothing caught

sindresorhus · 2016-11-29T08:18:14Z

lib/linux.js

+	const args = ['writable', 'org.cinnamon.desktop.background', 'picture-uri'];
+	// output = childProcess.execFile('gsettings', args, ());
+	return cp.execFile('gsettings', args)
+		.then(out => (out.toString().trim() === 'true'))


.then(out => out.trim() === 'true')

sindresorhus · 2016-11-29T08:19:03Z

lib/linux.js

-	if (/nitrogen/.test(app.cmd)) {
-		app.cmd = '/bin/sed';
+	if (app.cmd === 'nitrogen') {
+		const homeDir = require('os').homedir();


Don't require inline. require should be at the top of the file.

sindresorhus · 2016-11-29T08:20:18Z

lib/linux.js

+		const homeDir = require('os').homedir();
+		const configFile = path.join(homeDir, '.config/nitrogen/bg-saved.cfg');
+		return fs.readFile(configFile, 'utf8').then(config => {
+			config = config.toString().trim().split('\n');


.toString() is moot.

agreed, will fix

sindresorhus · 2016-11-29T08:23:41Z

lib/linux.js

+			config = config.toString().trim().split('\n');
+			const wallpaper = config.find(line => /file=/.test(line));
+			return wallpaper.replace('file=', '');
+		});


Could simply be:

return fs.readFile(configFile, 'utf8').then(config => config.trim().split('\n').find(line => /file=/.test(line)).replace('file=', '') );

I don't know what the format is like, but I assume you want to remove file= from the start of the line, so it should be /^file=/. Same with replace.

the config file has multiple lines, I just want the one with file=.
there can be whitespace leading file, so ^file would be invalid in that situation

I just want the one with file=.

That's already covered by the line splitting and .find.

here can be whitespace leading file

.trim() it before checking then. file= is too loose as if the filename was foofile=bar.jpg it would match. Quite unlikely, but I rather be safe.

really sorry about this, should've never happened

fa7ad · 2017-01-15T15:22:33Z

I think all the changes have been made, please re-check

sindresorhus · 2017-03-15T06:49:15Z

You need to fix the promise chaining for the set function too.

2.4.0

fa7ad · 2017-03-18T15:50:00Z

@sindresorhus should be okay now. Couldn't test it myself this time.

fa7ad · 2017-05-06T06:24:14Z

BUMP!

sindresorhus · 2017-05-06T06:40:38Z

@fa7ad I'm not super confident in the changes, especially since you haven't tested it, but I'll try to give it a thorough review.

fa7ad · 2017-05-06T13:40:02Z

@sindresorhus yeah sorry about that. I don't use GNOME or Cinnamon anymore. So, I'm not able to physically test it. Maybe someone who uses those systems can help out. But the changes are minimal and should work. Do give it a review though 😄

Add support for nitrogen on linux

5e716fd

This is popular among people who use tiling window managers (specially i3 users)

add save flag to make changes permanent

84ae82a

Fahad Hossain added 2 commits October 23, 2016 04:43

Add get method for nitrogen

de623e1

Temporary workaround for #18

fix the coding style issue

efe802b

fa7ad closed this Oct 23, 2016

fa7ad reopened this Oct 23, 2016

sindresorhus requested changes Nov 4, 2016

View reviewed changes

refactor the get method for nitrogen, fix #18

7125329

sindresorhus requested changes Nov 29, 2016

View reviewed changes

Fahad Hossain added 3 commits December 6, 2016 15:07

make requested changes #17

002c31e

fix promise chaining?

5a995dd

let -> const

45e0732

really sorry about this, should've never happened

mohammadrafigh mentioned this pull request Jan 7, 2017

Feature Request: Linux app please gilbitron/Splashify#1

Open

fa7ad mentioned this pull request Jan 15, 2017

Crashes on GNOME #18

Closed

Fahad Hossain added 3 commits March 18, 2017 20:58

Merge tag 'v2.4.0' into patch-1

d58ce4c

2.4.0

fix promise chaining for linux set method

1f2d990

fix npm test issus

72357f3

fa7ad mentioned this pull request Jun 2, 2017

Linux improvements #7

Closed

3 tasks

sindresorhus approved these changes Jun 8, 2017

View reviewed changes

sindresorhus merged commit d12f547 into sindresorhus:master Jun 8, 2017

		@@ -124,6 +130,10 @@ exports.get = function get() {

		const app = availableApps.find(app => app.get);

		if (/nitrogen/.test(app.cmd)) {

Add support for nitrogen on linux #17

Add support for nitrogen on linux #17

Conversation

fa7ad commented Sep 1, 2016

sindresorhus commented Sep 2, 2016

fa7ad commented Sep 2, 2016 • edited

fa7ad commented Sep 6, 2016

sindresorhus commented Sep 6, 2016

sindresorhus commented Oct 22, 2016

fa7ad commented Oct 22, 2016

fa7ad commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Nov 4, 2016

fa7ad commented Nov 4, 2016

sindresorhus commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fa7ad commented Jan 15, 2017

sindresorhus commented Mar 15, 2017

fa7ad commented Mar 18, 2017

fa7ad commented May 6, 2017

sindresorhus commented May 6, 2017

fa7ad commented May 6, 2017

fa7ad commented Sep 2, 2016 •

edited

sindresorhus Nov 30, 2016 •

edited