better handle color states at patch list#649
Conversation
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>
7f554de to
c46aab4
Compare
|
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). |
|
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/.
Despite being a compilation breakage, the LLM model wrongly classified it as |
|
Before this patch, the CSS style is broken, as it shows at most one <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 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 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 This way, the CSS style will properly apply the style for each .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 Note: this is just a brainstorming idea - didn't test if the above styles/css would work. |
|
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. |
For list, the patch is trivial:
I can certainly see its usage. On media, we prefer to hide |
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/.