This should fix this failure:
FAILED tests/test_snapshots.py::test_paint_character_picker_dialog[dark_unicode] - textual.css.query.NoMatches: No nodes match <DOMQuery query='CharacterSelectorDialogWindow'>
I noticed this first in a Windows VM, and am now seeing it in Ubuntu,
so it might have to do with the test running slowly.
This was back on textual 0.28.0 by the way; it doesn't have to do with
the recent updates (as far as I know; at least, not entirely.)
I've never had it reproduce when running in isolation with
pytest tests/test_snapshots.py::test_paint_character_picker_dialog
I tried adding a delay right before the query, and that DIDN'T work,
I got the failure at least once with that in place, so I think it was
failing to detect a double click, rather than querying while the window
was in the process of opening, and so I decided to try increasing the
double click threshold. The click() method of pilot has a cumulative
artificial delay of 0.3s, so two clicks is at least 0.6s and it's not
hard to imagine the event processing pushing that over 0.8s.
I actually created the `DOUBLE_CLICK_TIME` to allow overriding it in
tests, and I'm not sure if this actually works to override it.
I had to fix the layout of a few dialogs where elements decided they
wanted to start expanding a lot more than before.
I'm guessing this has to do with the changelog entry:
"Fixed relative units not always expanding auto containers"
https://github.com/Textualize/textual/pull/3059
The snapshot changes are basically bogus. The before and after are
visually identical, with the difference view showing all black.
Since there were a lot of switches to toggle and I had to wait for the
snapshot tests to run (slow!), I wrote a little automation to toggle
"Show difference" for all the results:
document.querySelectorAll("#flexSwitchCheckDefault").forEach((element)=> element.click())
It would be good to have this ability in the snapshot report UI itself,
maybe even replacing the individual toggles, although I'm not sure about
that, especially since it might be laggy toggling the blend modes with
a lot of test results. (I suppose if that was really an issue, it could
toggle all the visible test results and then toggle others as they come
into view, though that's a bit more complex.)
As for understanding the structural changes to the snapshots, I tried
making a visualization using hue, coloring according to the position
of a rect within the list of rects:
const richTerminals = document.querySelectorAll(".rich-terminal");
richTerminals.forEach(function(terminal) {
const rectElements = terminal.querySelectorAll("rect");
rectElements.forEach(function(rect, index) {
const fraction = index / (rectElements.length - 1);
const cycles = 40;
const hue = fraction * cycles * 360;
rect.style.fill = `hsl(${hue}, 100%, 50%)`;
});
});
This shows some difference, but it isn't very elucidating, since the
structural changes only show as gradual shifts in the hue, and affect
other rects even if said rects are identical, so it's subtle and messy.
Coloring based on a hash proves to actually highlight differences:
const richTerminals = document.querySelectorAll(".rich-terminal");
richTerminals.forEach(function(terminal) {
const rectElements = terminal.querySelectorAll("rect");
rectElements.forEach(function(rect, index) {
const hash = hash(rect.outerHTML);
const hue = (hash % 360 + 360) % 360;
rect.style.fill = `hsl(${hue}, 100%, 50%)`;
});
});
function hash(s) {
let hash = 0;
for (let i = 0; i < s.length; i++) {
const char = s.charCodeAt(i);
hash = (hash << 5) - hash + char;
}
return hash;
}
As for analyzing the differences now visible, eh, "maybe later."
There's no practical utility in this unless I want to make the palette size variable,
and this makes performance worse updating the screen when the palette changes,
but I'm on a mission to remove `from textual_paint.paint import PaintApp`
since it can't be at top level due to cyclic imports, and it has
side effects when importing `textual_paint.args` in turn.
My TODO comment said "add an attribute to ToolPreviewUpdate or make it's x/y Optional", which is both bad grammar ("it's" vs "its") and a bad idea, since the Canvas isn't involved in this update, so its event definition shouldn't have to be complicated by it.
I like this much better, just splitting the event handler into two functions, giving room to express the optional nature of the coordinate in the signature, and avoiding constructing two message objects at both of the callsites.
If git blame tools were smarter/easier to use, or if I was using a node-based programming paradigm, I would've never made this mistake. I would have done the natural and correct thing in the first place, but instead, I tried to avoid indenting a large block of code, which generates a noisy commit, and a barrier when using git blame.
