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

Implement italics support for Irssi #58

Merged
merged 2 commits into from Jul 7, 2014

Conversation

Projects
None yet
5 participants
@ailin-nemui
Contributor

ailin-nemui commented Jun 24, 2014

No description provided.

@dequis

This comment has been minimized.

Member

dequis commented Jun 24, 2014

While I appreciate the attempt to sneak 256 color support in an otherwise small pull request, I'm pretty sure this won't fool anyone.

...okay kidding aside: Does this actually depend on the 256 color patch? Or did those commits get included in the branch by mistake?

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jun 25, 2014

@dequis no this does not depend on 256 colours, however it is currently rebased on 256 colours, that's why github makes it show up like that. And unless there are strong urges to accept this before the other patches, I'm reluctant to change that. If you're concerned about review, either look at the individual patches or ailin-nemui/irssi@ailin-nemui:256-colour-history...italics

@ahf

This comment has been minimized.

Member

ahf commented Jun 27, 2014

We cannot merge this with the 256 colour support from here, so it would be nice if those patches could be removed from this patch set.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jun 27, 2014

@ahf actually it's the other way round ;-)

@@ -298,6 +298,9 @@ static int get_attr(int color)
if (color & ATTR_UNDERLINE) attr |= A_UNDERLINE;
if (color & ATTR_REVERSE) attr |= A_REVERSE;
#ifdef A_ITALIC

This comment has been minimized.

@ahf

ahf Jul 7, 2014

Member

Is this ever not defined? I see that we have no checks for these anywhere else.

@GeertHauwaerts

This comment has been minimized.

Member

GeertHauwaerts commented Jul 7, 2014

@ahf poke me if/when we merge this; I have a nice use case for this.

@ahf

This comment has been minimized.

Member

ahf commented Jul 7, 2014

Looks good to me! wberg also told me to land this.

@GeertHauwaerts This is coming in, now :-)

ahf added a commit that referenced this pull request Jul 7, 2014

Merge pull request #58 from ailin-nemui/italics
Implement italics support for Irssi

@ahf ahf merged commit 09a1801 into irssi:master Jul 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ailin-nemui ailin-nemui deleted the ailin-nemui:italics branch Jul 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment