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

[Bug]: 1.18.1 and later Regression in passing colors to graphs, fails to parse ${colorN} values #1469

Closed
madpilot78 opened this issue Mar 23, 2023 · 4 comments
Labels
bug Bug report or bug fix PR triage Issue that hasn't been verified

Comments

@madpilot78
Copy link
Contributor

What happened?

With v 1.18.1 the new code to handle colors (committed in 5e98c49) uses a different parsing logic for colors and will not recognize the ${colorN} options that it used to accept.

I thing this is a regression that should be fixed.

The behaviour I now get is that these colors are ignored and default ones get used. (red)

My first idea on how to fix it is to add code to parse_colour() to understand ${colorX} variables, by doing something like (pseudocode):

if (strncmp(name, '${color', 7) == 0) {
  /* parse name[7] as number and get the numbered color config, return that color */
}

Would such a patch be acceptable?

P.S. I'm running FreeBSD, which is not available in the OS selector

Version

1.18.1

Which OS/distro are you seeing the problem on?

Linux (other)

Conky config

(redacted, relevant parts)

color0 = "0D3E69",
color1 = "0057B3"

${cpugraph cpu0 32,0 ${color1} ${color0}}

Stack trace

No response

Relevant log output

No response

@madpilot78 madpilot78 added bug Bug report or bug fix PR triage Issue that hasn't been verified labels Mar 23, 2023
@bi4k8
Copy link
Collaborator

bi4k8 commented Mar 24, 2023

There was indeed a regression of color parsing in v1.18.1, but I believe it was fixed in #1447 which was released in v1.18.3.

As far as I'm aware, prior to v1.18.1, the syntax you provide parses incorrectly but while still allowing the graph to display in default colors rather than the specified ones. For example, the below conkyrc displays a white graph when tested against v1.17.0 and v1.18.0. Is there a conky version where you're seeing the ${colorN} variables actually get parsed within a ${cpugraph} variable? As far as I know, no code existed to do this.

conky.config = { 
    out_to_wayland = false,
    out_to_x = true, 
    alignment = 'middle_middle', 
    own_window_argb_value = 0, 
    own_window = false,
    default_shade_color = 'grey', 
    draw_shades = true, 
    font = 'Mono:size=13', 
    update_interval = 1,
    default_color = '#ffffff',
	color0 = "0D3E69",
	color1 = "0057B3"
} 
 
conky.text = [[ 
hello world
${cpugraph cpu0 32,0 ${color1} ${color0}}
]]

One workaround, which worked in older conky versions as well the current one is to use templates:

conky.config = {
...
  template1 = "00ff00",
}
conky.text = [[
${downspeedgraph wlan0 50,200 000000 ${template1} 1000 -t}
]]

@madpilot78
Copy link
Contributor Author

@bi4k8 It looked like ${colorN} was working for me in graphs, but now I think it was an idea I got by mistake. What most probably was happening was that it was using color0 as default color and displaying graphs in that one, which was indistinguishable from what I was expecting.

Although with 1.18.1 and higher it is now defaulting to red, which is different from what I expect. So my confusion.

I'm not sure what is the correct behaviour. The description of the syntax gave me the idea that using ${colorN} variables in graphs was acceptable, but if I' wrong I'll just change my configuration.

The reason I wrote this bug report is that I'm maintaining the FreeBSD port of conky and I have been holding back the update to 1.18.1 and later due to this change in behaviour, counting it as a regression.

Depending if it actually is a regression, and on the proposed solution I will decide what to do in the port.

@bi4k8
Copy link
Collaborator

bi4k8 commented Mar 24, 2023

The ${colorN} variables cannot be used inside of other variables to specify a color as an argument. Some Conky objects, including ${colorN}, are more like imperative commands than variables that expand to a value; when Conky encounters ${colorN} in the sequence of text and variables, conky changes the current color that it draws with, but it doesn't expand to the name of the color. As such, ${colorN} objects don't make sense to include as an argument to a ${downspeedgraph} object.

If the syntax description is misleading, we should update it to clarify the actual behavior.

The latest versions of Conky use red to highlight that the color is not what the user intended, and an error message is printed:

conky: can't parse X color '${color1}' (9)

@madpilot78
Copy link
Contributor Author

Thanks for the clarifications. I was clearly doing something wrong with my configuration.

I'm not sure I can state the documentation is wrong. It says nowhere that ${colorN} can be used as any other variable, it was an idea I got, and just by chance it partly worked so I thought I was right.

I'm closing this since there is no real regression. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report or bug fix PR triage Issue that hasn't been verified
Projects
None yet
Development

No branches or pull requests

2 participants