Skip to content

Avoid self-message highlights on IRC #193

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

Merged
merged 2 commits into from
Mar 6, 2017

Conversation

Sanqui
Copy link
Contributor

@Sanqui Sanqui commented Feb 26, 2017

When a user connected to both Discord and IRC speaks on Discord, they will likely get unwanted highlights in their IRC client. This is a fix which I believe is standard practice. By putting a zero width space character between each letter of the display name, we ensure IRC clients do not detect the name and highlight on it.

The other line in the commit is a fix where the wrong variable was being used.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.368% when pulling 99626a1 on Sanqui:avoid-highlights into 0f48dfe on reactiflux:master.

@Sanqui Sanqui force-pushed the avoid-highlights branch from 99626a1 to 8482a5e Compare March 1, 2017 20:53
@Sanqui
Copy link
Contributor Author

Sanqui commented Mar 1, 2017

I have cleaned this pull request up. It now uses zero width space characters which seem to work in all clients so far.

Copy link
Member

@ekmartin ekmartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean. I'll test it in an IRC client later and merge.


// Join the display name with double "bold" formatting characters
// to prevent unintentional highlights on the IRC side
displayUsername = displayUsername.split('').join('\u200d');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe merge this with line 164? I.e. let displayUsername = nickname.split('').join('\u200d');

Copy link
Contributor Author

@Sanqui Sanqui Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more explicit since it doesn't combine the operation with getting the name from another variable (and allows the comment to stand on its own), but I can do it.

test/bot.test.js Outdated
@@ -98,7 +98,7 @@ describe('Bot', function () {
};

bot.sendToIRC(message);
const expected = `<${message.author.username}> ${text}`;
const expected = `<${message.author.username.split('').join('\u200d')}> ${text}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add a simple helper function somewhere towards the top of the file that does the splitting?
i.e.

const escapeUsername = username => username.split('').join('\u200d');

@coveralls
Copy link

coveralls commented Mar 1, 2017

Coverage Status

Coverage increased (+0.02%) to 97.368% when pulling 6bfd774 on Sanqui:avoid-highlights into 0f48dfe on reactiflux:master.

@ekmartin ekmartin merged commit ebe340b into reactiflux:master Mar 6, 2017
@ekmartin
Copy link
Member

ekmartin commented Mar 6, 2017

Tested with Weechat, seems to be working well. Thanks! Released in version 2.2.0.

ekmartin added a commit that referenced this pull request Mar 12, 2017
This reverts commit ebe340b, reversing
changes made to 0f48dfe.
meooow42 pushed a commit to Reddit-R4R/discord-irc that referenced this pull request Jun 14, 2017
meooow42 pushed a commit to Reddit-R4R/discord-irc that referenced this pull request Jun 14, 2017
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.

3 participants