Tweak Quick Open theming

[MewPurPur]

Addresses some, but not all points in: godotengine/godot-proposals#11558

The things I've fixed are the issues with spacing and sizing.

Changed up the structure of list elements to prevent thumbnails from stretching vertically. Here's how this looked before:

Changed up the structure of grid elements too, to give some space above and to have better spacing. Here's how it looked before:

And there's now no spacing between elements, prevent you from accidentally clicking on the tiny space between.

I've also fixed the ugly hover style, which came from overriding the nonexistent "hover" style instead of "hovered".

Here's how it looks now:

Please tell me if you think the spacing is too tight now.

[Calinou]

Tested locally, it works as expected.

The amount of spacing looks good to me. However, the icons aren't centered in the hovered/selected area:

There are 6 pixels above the icon and 7 pixels below the icon. (The icon is 36 pixels tall.)
I can notice this issue across every item, so it's not specific to some items on the list.

I suggest adding 1 pixel of padding at the top so it's even.

[MewPurPur]

Sure. This is because the vboxcontainer with the text is 37px tall, because text has this tendence of taking more space than what it seems like it should.

I'll fix it.

[MewPurPur]

Doned, bumping up the "anti-separation" of the two labels by 1 more pixel looks good to me even with the maximum ascents/descents in the default font.

[Calinou]

Looks great and even now 🙂

It's still a tad uneven at 200% editor scale (10 pixels above the icon, 11 pixels below) but it's probably unnoticeable on an actual hiDPI display.

[MewPurPur]

Since the dialog redesign is new to 4.4, I think it'd be great to merge the theming fixes timely.

[MewPurPur]

Okay, I added SNAME where you suggested, but I honestly still don't get when it should be used and why it was suggested only for some strings. "separation" outside of the lines I changed also don't have it, "selected" next to my lines doesn't, and even "margin_*" constants in the lines I did change.

[arkology]

@MewPurPur I searched where else in codebase SNAME is used for specific strings (where specific strings are cached already and where is is used multiple times). Unfortunately, there is no consistency in it right now to make it clear where it should or should not be used, but as I see, if it was used multiple times in other places, it's ok to use it in new code.

[Ivorforce]

Here's what SNAME does: It statically assigns a StringName that will be referenced at this place for future use.
This is much faster than using String literals, but also forces the StringName data to be kept in memory. It is appropriate in places where the StringName data is expected to be kept in memory anyway, and especially when it is used in performance critical functions, like those called every tick.

I don't think there is an "official" recommendation for when to use it, but if i were to make one, I'd probably suggest using it for most static strings since most are expected to be kept in memory for one reason or another anyway. The cost of a reference to it is rather small.

[AThousandShips]

The general recommendation is to use it in hot paths, and not to use it everywhere. In modern cases speed usually wins over memory when prioritising but there's no use in doing it in paths that are called exactly once in a run

Note further that SNAME does not speed up the lookup itself, it just saves it for future use, so if you use it in a code path that's run exactly once it is no faster than just passing a string literal

[arkology]

@Ivorforce thanks for explanation. So actually "if it was used multiple times in other places" is not necessary, it is enough if it is already used in any "common" path (i.e. already cached).

[KoBeWi]

"Exactly once" is not that obvious.
For example
There is single EditorNode, so anything in it's constructor will run once.
However things in NOTIFICATION_THEME_CHANGED will run every time theme is modified. For one control it does not make difference, but for many of them it can affect how responsive is the editor when you change e.g. accent color.
There are multiple instances of QuickOpenDialog, but there is so few of them that SNAME is probably not worth it.
It's different for QuickOpenResultGridItem - there is lots of them per each dialog and SNAME in first item will cache the StringName for subsequent items, so it will make the dialog load faster.

[AThousandShips]

I think the important takeaway is that optimization (or any code pattern) should be applied with intentionality, with an understanding of why it's used, and that any pattern used "just because" is a code smell.

SNAME is a pretty harmless one but it's always good to get used to different patterns and why we use them and when

Edit: note to self to write some contributor documentation on some of this

[akien-mga]

Thanks!