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

Newsbeuter color08 is the same with color00 #186

Closed
pickfire opened this Issue May 21, 2015 · 16 comments

Comments

Projects
None yet
3 participants
@pickfire

pickfire commented May 21, 2015

I use color foreground color00 and color08. It seems that they are the same.

@h3xx

This comment has been minimized.

h3xx commented Jul 24, 2015

Can you provide more information? Such as:

  • What terminal are you using (xterm, rxvt, Terminology, Terminal.app?)
  • What is the actual contents of your config file (~/.newsbeuter/config)?
  • Are you using a terminal multiplexer like screen or tmux?
@pickfire

This comment has been minimized.

pickfire commented Jul 25, 2015

Can you provide more information?

Sorry, I forgot to specify the datails.

@h3xx

This comment has been minimized.

h3xx commented Jul 26, 2015

I looked at the code, and evidently using color08 knocks it back to "default".

Really, though, this makes sense because the escape sequence only really supports 0-7. \e[COLORm where color = 0-7 are the normal colors, then \e[01;COLORm means use the bold equivalent. "Gray" is just "bold black" as far as the terminal is concerned.

I managed to work around this by using the following line:

color background color00 color00 bold reverse

... or:

color background color00 color00 bold standout

This results in a gray (i.e. "bold black") background.

@pickfire

This comment has been minimized.

pickfire commented Jul 27, 2015

Really, though, this makes sense because the escape sequence only really supports 0-7.

But why can I use color18 and color19? It is still working as it is.

And it seems that the example you gave are also not working. I hope that newsbeuter can also support true colors as most of the terminals is already supporting it. And since ncurses have no support for it, newsbeuter won't have too. https://gist.github.com/XVilka/8346728

Maybe it have the capability of displaying 256 colors. Look at https://github.com/akrennmair/newsbeuter/blob/master/src/utils.cpp#L628

@pickfire

This comment has been minimized.

pickfire commented Sep 8, 2015

I looked at the code, and evidently using color08 knocks it back to "default".

What is the difference between default and black. Why shouldn't it work?

@Minoru Minoru added the to-check label Nov 23, 2015

@pickfire

This comment has been minimized.

pickfire commented Dec 13, 2015

@Minoru, thanks for helping newsbeuter to become active again.

I hope you can mark this as a bug as I had confirmed it with the git version.

@Minoru Minoru added bug and removed to-check labels Dec 13, 2015

@Minoru

This comment has been minimized.

Collaborator

Minoru commented Dec 13, 2015

Short story: when you write "color0N", that means that you're using octal number for the colour. "8" is not a valid octal numeral, so deep down in the libraries we use, "08" turns into "0"; that's why "color08" and "color00" are the same.

Long story: newsbeuter itself doesn't handle colours, it just passes them to STFL, our ncurses widgets library of choice. STFL, in turn, strips "color" prefix and then passes the remainder to wcstoul, which behaves the way described in the short story above. Unfortunately, STFL never checks if wcstoul consumed the whole string; that's why we aren't told of any errors when "08" is converted into "0".

That said, I believe we should sanity-check the values we read from the config. I'm not entirely sure about the approach yet (allow decimal, octal and hexadecimal? Restrict to decimal?), but it will be done. Stay tuned.

@Minoru Minoru closed this in 34d2392 Dec 13, 2015

@Minoru

This comment has been minimized.

Collaborator

Minoru commented Dec 13, 2015

Just pushed a fix. "n" in "color" is now restricted to decimal numbers that don't start with zero. Please test!

@pickfire

This comment has been minimized.

pickfire commented Dec 14, 2015

I think this patch fails color0, I need to use that for background. My color:

# Highlight
highlight article "^Title:.*" color4 color0
highlight article "^Feed:.*"  color3 color0
highlight article "^(Link|Date):[^\n]+" color19 color0
highlight article "https?://[^ \n]+" color1 color0

# Basic
color background              color7 color0
color listnormal              color7 color0
color listfocus               color18 color3
color info                    color19 color18
color article                 color7 color0
#color listnormal_unread       color07  color00
color listfocus_unread        color18 color3 bold
@Minoru

This comment has been minimized.

Collaborator

Minoru commented Dec 14, 2015

Yep, it does fail color0. And now that I think about it, this fix was really just a kludge. I'm working on a better one.

@Minoru Minoru reopened this Dec 14, 2015

@Minoru Minoru closed this in bec6ff3 Dec 14, 2015

@Minoru

This comment has been minimized.

Collaborator

Minoru commented Dec 14, 2015

New fix is here. Please test!

@pickfire

This comment has been minimized.

pickfire commented Dec 14, 2015

New patch for your fix. https://p.iotek.org/hf3

@Minoru

This comment has been minimized.

Collaborator

Minoru commented Dec 14, 2015

Look, I have no intention of making colours where N start with zero work. I've explained why. So your patch won't be merged, sorry.

@pickfire

This comment has been minimized.

pickfire commented Dec 15, 2015

I am not sure why you chose that decision even after I had read it my times.

If they chose color011 after many trial and error, isn't that the same as color11?

Wouldn't it be better for them to as the colors are still the same:

color background              color007 color000
color listnormal              color007 color123
color listfocus               color183 color234
color info                    color001 color008
color article                 color072 color000
color listfocus_unread        color018 color003 bold

Instead of

color background              color7 color0
color listnormal              color7 color123
color listfocus               color183 color234
color info                    color1 color8
color article                 color72 color0
color listfocus_unread        color18 color3 bold

It seems that I found highlight fails if 3 params were given and the error produced:

Error while processing command `highlight article "^Title:.*" color0' (/home/ivan/.newsbeuter/config line 79): `highlight' is not a valid color.
@Minoru

This comment has been minimized.

Collaborator

Minoru commented Dec 15, 2015

I am not sure why you chose that decision even after I had read it my times.

If they chose color011 after many trial and error, isn't that the same as color11?

Okay, let me explain one last time.

We used to just pass colours down to STFL. The library accepted numbers
in any radix, but of most interest to us is its ability to accept
octal numbers. To distinguish those from numbers with other radixes,
STFL (or, rather, wcstol()) looks for a leading zero. So: up until
recently, colours where number started with zero used octal system,
not decimal
.

So if someone used color011, they were really using not color11
but color9 (octal 11 equals decimal 9). (One consequence of this is
that if they put color011 in both Newsbeuter's and Mutt's configs,
for example, they will get different colours.)

#186 actually revealed a bug in STFL: it silently drops all numerals
that don't belong to current radix. Thus, when the library saw a colour
where number started with zero, it assumed radix eight and stopped as
soon as it encountered "8" or "9" in its input. That's why color00 and
color08 turned out to be the same—both are really just color0.

This bug is easy to work around in Newsbeuter, so I did. But it hinted
at the existence of a larger problem: colour numbers could be other than
decimal. Given that tutorials always deal with decimals (as far as my
experience goes), and a number of other programs (mcabber, tig) accept
only decimal inputs, this particularity of Newsbeuter's behaviour was
just an invitation for trouble. It became apparent that different
radixes should go, and decimals should be enforced.

That's easy enough to do, but there's a problem: decimals are often
padded with leading zeroes. If newer versions of Newsbeuter were to
strip leading zeroes, they would have started to misinterpret older
configs that used octals. Furthermore, they would do so silently, i.e.
user will see that colours changed, but Newsbeuter won't display any
errors or warnings. This will only happen to (an unknown) fraction of
user base, but it will happen.

That's why leading zeroes can't be ignored; Newsbeuter has to actively
prohibit their use in order to notify users who have octals in their
config about the change. They will see an error that they can (and
hopefully will) Google, find Newsbeuter's FAQ, and read about the change
and a way to fix their configurations.

Wouldn't it be nice for them to:

color background              color007 color000
color listnormal              color007 color123
color listfocus               color183 color234
color info                    color001 color008
color article                 color072 color000
color listfocus_unread        color018 color003 bold

It would, but:

  1. Up until now, it wasn't possible. So it's not like users lose
    anything; they just aren't gaining a feature they might've got under
    different circumstances.

  2. Instead of padding with zeroes on the left, they can pad with spaces
    on the right, like this:

    color background              color7   color0
    color listnormal              color7   color123
    color listfocus               color183 color234
    color info                    color1   color8   bold
    

    It doesn't look all too bad, does it?

Hope this explanation helped, and we can finally put the issue to rest.

It seems that I found highlight fails if 3 params were given and the error produced:

Error while processing command `highlight article "^Title:.*" color0' (/home/ivan/.newsbeuter/config line 79): `highlight' is not a valid color.

Now that's new. Let's file a separate bug for that!

@pickfire

This comment has been minimized.

pickfire commented Dec 15, 2015

Thanks for the explaination. Now I know that both will works:

Before
color11 -> yellow
color011 -> red (octal)

After
color11 -> yellow
color011 -> yellow FAILS!, so that the color will not gone wrong.

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