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

Fix for the graph scale text colour changing depending on graph position #1957

Merged
merged 2 commits into from Jun 6, 2024
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jun 4, 2024

Checklist

  • I have described the changes
  • All new code is licensed under GPLv3

Description

In the current latest code of conky, when you have a graph scale enabled, the text will change colour whenever a "high point" on the graph reached the "end" of the graph. It's difficult to describe so I took some screenshots:

Before

before-normal
before-error

The scale text changes colour to whatever the colour of the leftmost "bar" of the graph is.

After

after-normal
after-error

No longer does this

I'm not sure if this was an intended feature, if it was, feel free to close this. imo it shouldn't change colour because it makes the text very hard to read when the whole graph is full of red.
It's a simple fix, just adding one extra line to change the colour back to the "first" graph colour (i.e. the colour of the lowest values)

There shouldn't be any unintended side effects of the change, because looking at the code, this is the last if statement in a block, and right after the if statements, the colour gets changed anyway (set_foreground_color(last_colour);)

… scale, to ensure it always stays the same colour
Copy link

netlify bot commented Jun 4, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 776f3d6
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/665f31c498d3bd000887a46e

@github-actions github-actions bot added the sources PR modifies project sources label Jun 4, 2024
@ghost
Copy link
Author

ghost commented Jun 4, 2024

Not sure what the Docker issue is, I don't have any experience with Docker.

@brndnmtthws brndnmtthws requested a review from Caellian June 4, 2024 17:44
Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me, CCing @Caellian.

The docker build should be fixed in #1958 (hopefully for real this time).

Copy link
Collaborator

@Caellian Caellian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have approval from me as the change should work and the first_colour is a better choice than the one that fills the rect. And using the first column for coloring is wrong anyway (should be current state?).

There are edge cases with readability still - in case someone uses a gradient that's got similar color stops (e.g. lightness gradient with 40% difference). But the colors might also be similar to last_colour (borders), so there's really no perfect solution that will work in all cases and make everyone happy with how it looks.

Thank you for the contribution, it's a good improvement. Feel free to merge it if you don't plan on making any further modifications.

@brndnmtthws brndnmtthws merged commit f88b50d into brndnmtthws:main Jun 6, 2024
62 of 63 checks passed
@brndnmtthws brndnmtthws added the bug Bug report or bug fix PR label Jun 6, 2024
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 sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants