Skip to content

better handle color states at patch list#649

Merged
stephenfin merged 1 commit into
getpatchwork:mainfrom
mchehab:better-color-checks
Jun 7, 2026
Merged

better handle color states at patch list#649
stephenfin merged 1 commit into
getpatchwork:mainfrom
mchehab:better-color-checks

Conversation

@mchehab

@mchehab mchehab commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

The logic which colors a check is weird: it only add the right class for up to one column.

That's weird and makes harder for people to properly idenfify CI checks when there are multiple CI reports at the same line.

Change it to do the expected behavior, like can be seen at https://patchwork.linuxtv.org/project/linux-media/list/.

@mchehab mchehab changed the title patch.py: better handle color states better handle color states at patch list Jun 6, 2026
@stephenfin stephenfin changed the base branch from stable/3.2 to main June 7, 2026 15:14
The current logic applies checks color only once. That's
weird and makes harder for people to properly idenfify
CI checks when there are multiple CI reports at the same line.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
@stephenfin stephenfin force-pushed the better-color-checks branch from 7f554de to c46aab4 Compare June 7, 2026 15:14
@stephenfin stephenfin merged commit dbffab9 into getpatchwork:main Jun 7, 2026
11 checks passed
@alialnu

alialnu commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

@mchehab, @stephenfin,

The single-color behavior was intentional. When this was added in 974ff0c, the goal was to make the worst non-zero state stand out: only the highest-priority count (Fail, then Warning, then Success) gets a color so failures and warnings aren’t diluted by also highlighting lower priority badges on the same row.

Coloring every non-zero count makes mixed rows easier to read, but it removes that emphasis (e.g. a red failure no longer dominates when success and warning counts are also colored).

@tmonjalo

tmonjalo commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

I disagree with changing this behaviour. When changing such behaviour, you should propose a configuration option at project level to choose which behaviour we want.

@mchehab

mchehab commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

I disagree with changing this behaviour. When changing such behaviour, you should propose a configuration option at project level to choose which behaviour we want.

coloring just (up to one) CI report is confusing, and doesn't really work when there are lots of CI reports. On media, it is not uncommon to have error reports with all tree error codes like this one:

and we do want to have the colors at the patch list matching the ones displayed at the checks.

Besides that, with LLM-based bots like Sashiko, one can't really say if a report is a warning or an error.

See for instance this one, and the corresponiding CI analysis at: https://lore.kernel.org/all/20260530114225.869391F00893@smtp.kernel.org/.

  • [Low] Intentional compilation breakage via an invalid syntax token at global scope.

Despite being a compilation breakage, the LLM model wrongly classified it as Low priority, probably just because the patch describtion told that this was expected.

@mchehab

mchehab commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Before this patch, the CSS style is broken, as it shows at most one state:

<td class="text-nowrap">
    <span title="Success / Warning / Fail">
        <span class="patchlistchecks">5</span>
        <span class="patchlistchecks">2</span>
        <span class="patchlistchecks fail">6</span>
    </span>
</td>

which is broken, as the stateis missing for the first two span classes.

With this patch, what we have is that each status will be placed at the span class:

<td class="text-nowrap">
    <span title="Success / Warning / Fail">
        <span class="patchlistchecks success">5</span>
        <span class="patchlistchecks warning">2</span>
        <span class="patchlistchecks fail">6</span>
    </span>
</td>

This is a CSS coding syle fix. as now state is properly mapped.

Btw, this is coherent to what happens at the Checks table inside the patch view, where the state is always present at the checks <spam> class.


Now, what you want is to have a way to apply a different style to the highest state.

Maybe there is a way to support both scenarios by changing the html output to something like this, at the patch list:

<td class="text-nowrap">
    <span title="Success / Warning / Fail">
        <span class="patchlistchecks success">5</span>
        <span class="patchlistchecks warning">2</span>
        <span class="patchlistchecks fail highest">6</span>
    </span>
</td>

e.g. adding an extra highest parameter to the css style to indicate the highest priority state.

This way, the CSS style will properly apply the style for each state with (I simplified the css style here for the sake of making it shorter):

.patchlistchecks.success { color: green; }
.patchlistchecks.warning { color: yellow; }
.patchlistchecks.fail { color: red; }

Sites that want to ignore the color except for the highest priority would do, instead:

.patchlistchecks.success.highest { background-color: black; color: green; }
.patchlistchecks.warning.highest { background-color: black; color: yellow; }
.patchlistchecks.fail.highest { background-color: black; color: red; }

It would even be possible to have both CSS styles altogether, if one wants to keep colors and still highlight the highest priority.

Now, for coherency, the same way as the highest attribute would be used on patchlistchecks for list view, on patch view, a highlight attribute would be needed for checks class.

Note: this is just a brainstorming idea - didn't test if the above styles/css would work.

@tmonjalo

tmonjalo commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Yes we can have a new class highest, it looks a good idea.

About the usage, in DPDK we don't classify AI review higher than warnings and we want to colorize only the highest problem so we see easily if a patch is really failing. The idea to have only one color is that it shows the overall status of the patch.

@mchehab

mchehab commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Yes we can have a new class highest, it looks a good idea.

For list, the patch is trivial:
mchehab@fc6379a

About the usage, in DPDK we don't classify AI review higher than warnings and we want to colorize only the highest problem so we see easily if a patch is really failing. The idea to have only one color is that it shows the overall status of the patch.

I can certainly see its usage. On media, we prefer to hide - except when no checks, and preserve the same colors. Anyway, with the higherst approach, either way can be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants