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

Stopgap solution to preserve colors and attributes on highlighting #1542

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

C0rn3j
Copy link
Contributor

@C0rn3j C0rn3j commented Sep 29, 2024

Old demo with wrong column highlights
Screencast_20240929_172318.webm
Screencast_20240929_181531.webm

This is a decent workaround for #243 solely by using text attributes by taking the idea in #434 and expanding it.
This is a stopgap solution until the issue is properly resolved by actually being able to define the desired text colors while keeping the highlight background, which I assume will take a while.
This does not have the same problem as #434 as we are not changing text color by bolding it, we are primarily relying on the terminal to do its highlighting by means of A_STANDOUT.

This works for any highlight color.

There is a couple problems:

  • A Function is from original PR and needs renaming: RichString_setAttrn_preserveBold -> RichString_setAttrn_preserve || RichString_setAttrn_preserve_with_standout ?
    Settled on _preserveWithStandout for now
  • There are debug statements left in the current code
  • I am not a C/C++ dev, someone please verify my actual functional code makes sense
    • Currently I check if text color is not default (ColorPair 0) or if text is Bolded, is there even a situation where text is bolded but not colored?
  • The entire column gets lit up - is this desired? I presume not.
    • A) Should I solve this in my code by ensuring the char is not a space -> I fixed it by doing that in the current code.
    • B) Should this be fixed in other code that ensures it's not setting params and text colors for spaces?
      image
  • Biggest thorn is the fact that we can be setting highlight color that is identical to the text color, which means we are not highlighting anything different from default text.
    • This is detected and text attribute is set to italics to at least differentiate it, but it is not an amazing solution, we would need to at least italicize the entire column for it to not look poorly(I presume checking chars ahead and behind until whitespace is found and italicizing those would work):
      image
    • See my comment in Preserve colors/highlighting for highlighted lines #243 for a list of attribute tricks if you want to try to wizard a better solution, underline looks terrible and bolding has the same issue as italics + color issues
    • This could be solved by not having the theme use the same text color as the highlight color
  • Does this work fine in most terminals? They need to treat A_STANDOUT well from what I understand.

Mildly related to #1541

@BenBE
Copy link
Member

BenBE commented Sep 29, 2024

I converted this PR to a draft right now, as there are several issues I see with this current implementation.

  1. Visually this does not look right on several levels and I think we should discuss some of the workings for the themes to find a good workaround for those. E.g. for the highlighting to the right (red+yellow for basenames) the coloring inversed just looks ugly and much too offending to the eye.

  2. Please keep the git history short and linear with corrections squashed/fixup'd into the erroneous commit.

  3. Several build failures on the CI. If you need help with addressing these, feel free to ask.

Regarding highlighting there are basically 5 options right now:

  • Normal
  • Selected
  • Followed
  • New Process
  • Terminated Process

On your question with A_BOLD without color: AFAIR there's no place that uses this and if it is used, the most likely place to look would be with the (text-styled) meters.

@BenBE BenBE added the enhancement Extension or improvement to existing feature label Sep 29, 2024
@C0rn3j
Copy link
Contributor Author

C0rn3j commented Sep 29, 2024

On your question with A_BOLD without color: AFAIR there's no place that uses this and if it is used, the most likely place to look would be with the (text-styled) meters.

Thanks, threw that check out.

Please keep the git history short and linear with corrections squashed/fixup'd into the erroneous commit.

Edits are enabled, I assumed that whoever pulls it squashes it into a single commit.

Several build failures on the CI. If you need help with addressing these, feel free to ask.

Fixed all but one, help welcome.

It was pain to actually find the issues in the walls of text.
If the tooling can be forced to use colors in the CI (CI itself can handle colors), it would be nice if it did.
Looks like it should be possible?

Visually this does not look right

I know, though I do not intend it to look perfect, just good enough for normal use until proper colors are implemented.

If the suggested variant is not acceptable, some others would be:

A) A_STANDOUT + retain A_BOLD for all colored text:
image

B) A_STANDOUT + retain A_BOLD for all A_BOLD text:
image

C) Just don't touch BOLD text:
image

D) Just don't touch color text:
image

My main goal here is to retain the yellow/red highlighting as I rely on it often and when I filter out instead of search for the process I wish to know is misbehaving because of outdated libs/binary, I get no info about it, this helps:

image

@BenBE
Copy link
Member

BenBE commented Sep 29, 2024

On your question with A_BOLD without color: AFAIR there's no place that uses this and if it is used, the most likely place to look would be with the (text-styled) meters.

Thanks, threw that check out.

NP.

Please keep the git history short and linear with corrections squashed/fixup'd into the erroneous commit.

Edits are enabled, I assumed that whoever pulls it squashes it into a single commit.

Usually we aim to touch the PRs as little as possible. The PR itself should already tell the changes; the story how you got there is of little interest. Have a look at the style guide for some details.

Also, with your code, please mind the indentation: We don't do hanging indentations, but instead do either not break (reasonably long lines allowed) or indent exactly one more level. In the two implementations for RichString_setAttrn_preserveWithStandout a long line on the ch = … ?: line would be acceptable.

Several build failures on the CI. If you need help with addressing these, feel free to ask.

Fixed all but one, help welcome.

Will take a look …

It was pain to actually find the issues in the walls of text. If the tooling can be forced to use colors in the CI (CI itself can handle colors), it would be nice if it did. Looks like it should be possible?

Will take a look …

Visually this does not look right

I know, though I do not intend it to look perfect, just good enough for normal use until proper colors are implemented.

If the suggested variant is not acceptable, some others would be:

A) A_STANDOUT + retain A_BOLD for all colored text: image

B) A_STANDOUT + retain A_BOLD for all A_BOLD text: image

C) Just don't touch BOLD text: image

D) Just don't touch color text: image

My main goal here is to retain the yellow/red highlighting as I rely on it often and when I filter out instead of search for the process I wish to know is misbehaving because of outdated libs/binary, I get no info about it, this helps:

image

The black background for places where colors were preserved visually makes this kinda noisy. So, for most of the columns (e.g. sizes) mapping the color to e.g. bold would usually already suffice and would be much less distracting. Yes, that's not very helpful with your usecase for the outdated binary highlighting while following, but this at least would be a good first step.

For the second step, we may extend the color stuff in a way to allow for the (internal) attributes mapped to the theme colors contextually (e.g. specify bold black with red background for a removed process with outdated executable, but bright red with orange background if you followed the same process. But this will likely need some updates to the theme data and thus should be postponed for a bit.

@BenBE
Copy link
Member

BenBE commented Sep 29, 2024

The remaining error on OpenBSD is this one:

cc -DHAVE_CONFIG_H -I.  -DNDEBUG  -std=c99 -pedantic  -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wextra-semi-stmt -Wimplicit-int-conversion -Wnull-dereference -Werror -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./openbsd" -g -O2 -MT RichString.o -MD -MP -MF $depbase.Tpo -c -o RichString.o RichString.c &&\
mv -f $depbase.Tpo $depbase.Po
RichString.c:190:24: error: use of undeclared identifier 'A_ITALIC'
         attrToPass |= A_ITALIC;
                       ^
1 error generated.

Issue seems to be, that A_ITALIC is not supported in the ncurses version of OpenBSD or needs to be treated differently as A_ITALIC is an extension.

This tries to keep special markup like large numbers and
other special markup inside a process row even when such
a line has been highlighted by being selected, followed,
or otherwise marked in the list view.

Alternate take on PR htop-dev#434.
@C0rn3j
Copy link
Contributor Author

C0rn3j commented Sep 29, 2024

So, for most of the columns (e.g. sizes) mapping the color to e.g. bold would usually already suffice and would be much less distracting

Bolding things will have terrible contrast in some cases, which was done and was refused in the last PR.

Issue seems to be, that A_ITALIC is not supported in the ncurses version of OpenBSD or needs to be treated differently as A_ITALIC is an extension.

I'll ifdef it somehow if it will even be kept in the end, and just not have it on OpenBSD I suppose.


Here is my least intrusive idea so far - just A_STANDOUT everything that's not colored:

Screencast_20240929_234724.webm

image

None of this really looks great overall, but it's not terrible either.
New/dead processes having colored numbers looks the worst.

As a side note, I am really digging how this soft-highlight looks like compared to the full-on background highlight in current htop.
It could look really good when done properly and we get to just change the background while keeping text properties, as opposed to blasting everything with the same color.

EDIT: Took the time, it can now be solved properly, but it needs design changes.

#243 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Extension or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants