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

irc: don't consume ',' in mirc color code if missing foreground color #1623

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chzchzchz
Copy link

"To specify a background color, a foreground color has to be given. So a
^C,8 attribute is not valid and thus ignored."

Chomping the ',' will corrupt some mirc color scenes.

"To specify a background color, a foreground color has to be given. So a
^C,8 attribute is not valid and thus ignored."
@codecov-io
Copy link

Codecov Report

Merging #1623 (d400110) into master (4f606ce) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1623   +/-   ##
=======================================
  Coverage   35.24%   35.24%           
=======================================
  Files         202      202           
  Lines       82680    82681    +1     
=======================================
+ Hits        29137    29138    +1     
  Misses      53543    53543           
Impacted Files Coverage Δ
src/plugins/irc/irc-color.c 93.26% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f606ce...d400110. Read the comment docs.

@flashcode
Copy link
Member

Hi,

Thanks for the PR.

The new behavior is more strict, is it a problem to evaluate codes even if they are considered as invalid?
This could now break if people relies on the fact that WeeChat properly handles them.

Is there some official specifications for these codes? (as far I know there is not, in which case allowing such code could be OK for me).

@flashcode flashcode self-assigned this Mar 27, 2021
@flashcode flashcode added the bug Unexpected problem or unintended behavior label Mar 27, 2021
@chzchzchz
Copy link
Author

Quote is taken from https://www.mirc.com/colors.html which is probably the closest thing to canonical. irssi displays the ',', irccloud eats ',' and does bg update + fg reset, weechat eats ',' and bg update. This change will fix the ascii I scroll that tests whether color handling is broken on peoples' clients.

@ailin-nemui
Copy link

Irssi did an investigation about this topic here irssi/irssi#742 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants