Solarized line number wrong colors

[junpet]

The line number column has wrong colors (both font and background color).
Solarized homepage

Solarized dark

Current font color for line numbers: base0 #839496
Font color should be for line numbers: base01 #586e75
Current background color for line numbers: not a Solarized color #f0f0f0
Background color should be for line numbers: base02 #073642

Solarized light

Current font color for line numbers: base00 #657b83
Font color should be for line numbers: base01 #586e75
Current background color for line numbers: not a Solarized color #f0f0f0
Background color should be for line numbers: base2 #eee8d5

[pfertyk]

Please correct me if I'm wrong, but isn't background color for line numbers hardcoded?

        if nocls:
            yield 0, ('<table class="%stable">' % self.cssclass +
                      '<tr><td><div class="linenodiv" '
                      'style="background-color: #f0f0f0; padding-right: 10px">'
                      '<pre style="line-height: 125%">' +
                      ls + '</pre></div></td><td class="code">')
        else:
            yield 0, ('<table class="%stable">' % self.cssclass +
                      '<tr><td class="linenos"><div class="linenodiv"><pre>' +
                      ls + '</pre></div></td><td class="code">')

Also, these seem to be the only 2 occurrences of the class linenodiv. I've generated a css file using pygmentize -S solarized-dark -f html and I didn't find any classes responsible for line numbers. Could you point me in the right direction?

[Anteru]

Right, currently it's hardcoded. That's the whole problem -- two colors from the styles need to get picked (which would work well as line colors) and then used for the lines. Which means finding out which colors are suitable in the first place (and/or compute the from one color somehow...)

[junpet]

I didn't know it's hardcoded. It was similar color, I thought there was some error.

On the webpage you can find codes for line numbers and backgrounds. But you can find them in my previous comment as well.

[pfertyk]

I can see that most styles are associated with Pygment tokens, except for background_color and highlight_color. I think it would make sense to treat the line background and font in the same way as these two (separated from tokens). I'll modify the Styles class: it will try to get one of the existing values (e.g. duplicate background_color for line numbers background and normal text token color for line numbers text) so we don't have to change anything in the existing styles, but it would be possible to specify the value manually if needed.

I'll send and update once I have some working code ;)

[pfertyk]

I've added 2 new fields to Style, one for font color, one for background color. It seems that Pygments defines Solarized colors separately for light and dark versions, so I checked the proper color values for both and used them (e.g. I used LIGHT_COLORS['base02'] instead of just base2 suggested in the description of this issue).

I still need to add tests and a better default value for the font color. Then I'll refactor the HTML formatter to actually use the new values instead of the hardcoded ones.

[pfertyk]

I've moved all of the styles from CSSFILE_TEMPLATE to get_style_defs, which seems to make more sense and allows using the style to get the proper color values. I've amended the tests and added new ones, to check if the new fields from the Style class are taken into account.

I've noticed that line number styles are not used the same way as other styles. They don't use the CSS prefix, because they are not included in the highlight div. I've kept it that way, please let me know if this was the right decision.

I've noticed there are still some occurrences of #f0f0f0 color, which are used for inline styles (option noclasses=True). I will try to get rid of those too, replacing them with colors from the current style.

One question I'd like to ask @Anteru : there is an option to define "special" line numbers (linenospecial). Normally, those line numbers get the special class, but with inline styles they also get a predefined background color `#ffffc0:

            if sp:
                for t, line in lines:
                    if num % sp == 0:
                        style = 'background-color: #ffffc0; padding: 0 5px 0 5px'
                    else:
                        style = 'background-color: #f0f0f0; padding: 0 5px 0 5px'
                    yield 1, '<span style="%s">%*s </span>' % (
                        style, mw, (num % st and ' ' or num)) + line
                    num += 1

Should this color also be defined as part of the style, like the normal line number background and font colors? It is currently defined only for inline styles, not for the special class, was that intentional?

[Anteru]

I have no idea if that was intentional (maybe @birkenfeld remembers), but from my point of view there should be no hardcoded colors. At the very least they should come somewhere from a variable, but ideally they're part of the CSS.

[birkenfeld]

That was probably an initial value and got overlooked as a necessary style property when switching to styles.

[pfertyk]

I've added another attribute to Style class: line_number_special_background_color. It will be used for special line background. I've noticed that special lines don't have a different font color, so I didn't add an attribute for that. The Style class has now a bit more attributes that it had before, but I think this is the best solution in this case, so that we can keep everything in the style. Please let me know if you have other ideas, I'd love to discuss them.

With the special background color moved to Style, I could get rid of all hard-coded colors in the HTML formatter. I've added some methods to return formatting of different elements (pre and line number related elements), so that styles are defined only in one place. I've also moved the inline style for linenos class to the td element (it was added to the div element), so the style can be consistently applied for normal and inline styles.

The last of those changes revealed that the table style for line numbers doesn't look very well by default, so I'll either revert it or try to fix it. I'll also need to add a lot of new tests, to make sure that line number styles and special line number styles are applied for inline and normal styles.

This was supposed to be a really small task, but it turned out to be a big piece of refactoring ;) I don't mind finishing this, I think most of the job is done, now I just need some smaller refactoring and a lot of new tests. Let me know if that new, larger scope of this task is OK with you.

[pfertyk]

I've added styles for special lines for linenos=table, it seems they didn't work before (unless I've missed something). I've noticed that the documentation for noclasses option only mentions the token span tags, but it worked for line numbers too, both for table and inline (at least for non-special lines). If there are no objections, I will update the documentation to mention also the line number styles.

[Anteru]

Yes, I'm fine with this thorough fix -- and in hindsight "good first issue" may not have been the best label here. I do appreciate trying to fix this properly. Regarding the doc updates, please go ahead.