Strip color codes from notifications #634
Conversation
|
This should probably strip all control codes, not just colors.
|
|
Hey @dgw thanks for the suggestion. Initially I thought it wouldn't be necessary to handle these, as they represent characters that won't be displayed, but I suppose it depends on the situation. |
|
If only worrying about characters that are displayed, there's no reason to remove Thanks for updating the PR. I figured (and should have explained initially) that it's best to remove all of the control codes in case of misbehaving fonts, or other weirdness that could arise from having non-printing characters in the notifications. There are a lot of system configurations out there, and as long as we're stripping one kind of control character… 💯👍 |
|
👍 However, this probably conflicts #627 as that PR moves code around. |
| @@ -655,7 +655,7 @@ $(function() { | |||
| favico.badge("!"); | |||
| if (settings.badge && Notification.permission === "granted") { | |||
| var notify = new Notification(msg.from + " says:", { | |||
| body: msg.text.trim(), | |||
| body: msg.text.trim().replace(/\x02|\x1D|\x1F|\x16|\x0F|\x03(?:\d{1,2}(?:,\d{1,2})?)?/, ""), | |||
There was a problem hiding this comment.
Perhaps it should trim after replace?
There was a problem hiding this comment.
Yeah, thanks for noticing that. Will make suggested change.
There was a problem hiding this comment.
While you're at it, change \d to [0-9]. See http://stackoverflow.com/a/6479605/2200891
|
My apologies for the number of commits this is taking for a single regex! I haven't made much contribution to open source projects before, so thank you for being so helpful along the way. |
|
Don't worry about that, @sprusr, we all have to start somewhere :-) The only thing is that before this gets merged, it'll be good to squash the commits into 1. |
Fixes #628