Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rendering \n as a newline for Darwin-NotificationCenter implementation #43

Closed
wants to merge 1 commit into from

Conversation

thataustin
Copy link
Contributor

Hey, I'm wondering if you'd be interested in allowing \n to appear as actual line breaks for any of the platforms you're supporting.

Here's how the last example notification from test.js appears with these changes.

Example of changes using test.js

@tj
Copy link
Owner

tj commented Jan 17, 2014

why not use a real newline? not sure I get it

@tj
Copy link
Owner

tj commented Jan 17, 2014

I think it's better to leave this to the program passing the string

@thataustin
Copy link
Contributor Author

The problem is that the message string is getting JSON.stringify'd, so (afaik) it's not possible to send an actual newline through to terminal notifier because stringify turns it into \n, which escapes the slash needed to represent the newline. I might be missing something, though. Do you know how to create visual line breaks in messages?

@sgtpep
Copy link

sgtpep commented Mar 24, 2014

@visionmedia, please, accept this pull request. There is no way to display the message with real new lines using your package without ugly monkey patching.

@technobly
Copy link

Are you sure JSON.stringify() is messing up the '\n' ? Look what happens in Chrome Console:
yo yo

@sgtpep
Copy link

sgtpep commented Mar 24, 2014

@technobly It becomes not single symbol representing new line, but two characters: backslash and letter 'n'. Shell does not automatically expand '' and 'n' to newline inside quoted strings.

@technobly
Copy link

How about now?

@sgtpep
Copy link

sgtpep commented Mar 24, 2014

This string goes to shell. So correct example would be:

$ echo "foo\nbar"
foo\nbar

The node-growl puts stringified "foo\nbar" to command that starts looking like:

growlnotify -m "foo\nbar"

And it gets literal appearing of message with '' and 'n'. That's how shell quoting works.

@technobly
Copy link

Ahh, I see. But if that's true, why is @visionmedia suggesting a "real newline" and "I think it's better to leave this to the program passing the string" and how could one do that?

Is he suggesting you just do this:

growl('here \' are ' + '\n' + 'some \\ characters that " need escaping');

@sgtpep
Copy link

sgtpep commented Mar 25, 2014

Just tried: same result:
screen shot 2014-03-25 at 16 36 58

@technobly
Copy link

I guess you keep coming back to the point that the Shell is screwing things up. I keep banging on the fact that this should work fine from a javascript app. Do you really use it from the Shell? Well I guess that's not a valid question, of course you do :) So it seems pretty simple to me, all strings are going to need some extra massaging.

Just 2 months ago, there wasn't even a JSON.stringify() in the code, just a .replace() function so I don't see why there might be resistance to add some of that back. @visionmedia would you please share your thoughts on this?

@sgtpep
Copy link

sgtpep commented Mar 25, 2014

The last screenshot does not exact shell, it's node console. I just wanted to demonstrate the actual result of the execution of your last code snippet.

@technobly
Copy link

It works ok for me, however the escaped backslash doesn't work:
yo

Also directly in growlnotify works great with your above example...
yo

Also without quotes:
yo

Also directly from the test.js file:
yo

I can't however make that backslash work, no matter what I try it's always doubled, or missing.

@sgtpep
Copy link

sgtpep commented Mar 26, 2014

Seems like the bug only appears on *nix platforms. Hope to see it woking on all of them someday.

@technobly
Copy link

BTW this fixes the backslash problem:

// Line 226:

    case 'Windows':
      //args.push(quote(msg));
      // escaping fix
      var stringifiedMsg = quote(msg);
      var escapedMsg = stringifiedMsg.replace(/\\\\/g, '\\');
      args.push(escapedMsg);
      // end fix
      if (options.title) args.push(cmd.title + quote(options.title));
      break;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants