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

Monitors colors+percentages+minor adjustments #7

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

Conversation

jjustra
Copy link

@jjustra jjustra commented Jan 17, 2020

Hello, I've played with 'monitors' and made some changes. Hope they will be helpful :)

@jjustra jjustra changed the title colors+percentages+minor adjustments Monitors colors+percentages+minor adjustments Feb 4, 2020
@LStranger
Copy link
Member

Thank you very much. Will consider it for version 0.11.0.

@ib
Copy link
Member

ib commented Aug 5, 2023

For the most part, the patch is okay, but there are a few details that need to be addressed.

  • You changed the code (and instructions) by adding Txt and Bg colors. Configurable Bg seems fine, but what if the user doesn't want text? Text (yes/no) should be configurable, and Txt color should only appear (or be editable) then.

  • Don't change default values (DEFAULT_WIDTH, default_colors) unnecessarily, especially if they are configurable anyway.

  • Don't add dead code and commented code (text_size) that you don't use.

  • Don't use fixed size buf[] and sprintf()! There is g_strdup_printf().

  • Make "%.0f%%" translatable.

  • I suppose the positions in cairo_move_to() depend on the font size. If not configurable, define the font size as a macro and use it for the calculation.

  • Nitpick: A lower case txt and bg in CPUtxtColor, RAMbgColor etc. is IMHO easier to read.

I like the idea of showing percentages, but you should probably split the patch step by step (make a patch series): add background color (with a decent default, please!, your default must not change the current default), add text (configurable: yes/no), add text color (could be selectable, i.e. text color implies that the user wants text), add text_size, and so on. This way it should be easier and we can merge piece by piece, even if you are not quite finished, because each patch is self-contained.

@ib ib added the question Further information is requested label Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants