Always render checkbox/radiobutton background
ClosedPublic

Authored by ndavis on Fri, Jan 10, 7:30 PM.

Details

Summary

QQC2/Kirigami checkboxes and radio buttons can turn invisible when rendered over a selected item because their background isn't rendered and they don't have any hacks to detect when they're being rendered over a selected list item.

While the problem isn't technically a Breeze QStyle problem and a hack could be made for QQC2/Kirigami, I don't think there's any great style benefit to not rendering a background for the checkbox. I suppose there is a performance benefit to not rendering a checkbox background except for when the background is different from normal. In my testing with GammaRay's paint analyzer, the checkbox background has a cost less than 5%. The radiobutton background has a cost of 15-20% (maybe it can be improved?).

Diff Detail

Repository
R31 Breeze
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ndavis created this revision.Fri, Jan 10, 7:30 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Jan 10, 7:30 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ndavis requested review of this revision.Fri, Jan 10, 7:30 PM
ndavis updated this revision to Diff 73234.Fri, Jan 10, 7:48 PM

remove .clangd/

I would prefer not systematically render the background, because it might break existing applications, like rendering a widget on top of a painted image. Also this goes against Qt's rendering model which does not require rendering widget background, relying on that of the parent widget or window itself. I would instead either do it in qqc, or detect if we are rendering a qqc control and in this case only render the background systematically. There are several places in the code that do that already. Look of "#if BREEZE_HAVE_QTQUICK" blocks.

Hugo

I would prefer not systematically render the background, because it might break existing applications, like rendering a widget on top of a painted image.

In fact that exact use case is already broken by the lack of a built-in background. See the wallpaper slideshow view:

Hugely improved visibility with this patch:

ndavis added a comment.EditedMon, Jan 13, 11:47 AM

Since this improves checkbox visibility when rendered over images, should we or should we not do this? Is there a way to detect when a checkbox is rendered over an image? Is the added complexity of making all these exceptions better than always rendering the background?

ngraham accepted this revision as: VDG, ngraham.Mon, Jan 13, 2:08 PM

Purely in terms of visuals, I think so. It clearly fixes a bug.

This revision is now accepted and ready to land.Mon, Jan 13, 2:08 PM
hpereiradacosta accepted this revision.Mon, Jan 13, 3:11 PM

ship it. No strong feeling.

This revision was automatically updated to reflect the committed changes.

Oh, I should have brought this up before landing, but any opinions about always using QPalette::Base (View Background) for the background color? I asked on the VDG channel and people seemed to be unanimously in favor of using View Background as the background color.

I would strongly object to that:
first that is a visual change unrelated to the issue this patch adress
second this is semantically wrong, and this will break on some color theme.
Things should be kept consistent. If you change the background role, you must also change the foreground role, and then you might really get funny results.

hpereiradacosta added a comment.EditedMon, Jan 13, 5:09 PM

in more detail: imagine a color scheme where window background is black, window text is white, view background is white view text is black.
you will get a white square on a white background for unchecked checkbox ...
I would really revert this part of the change

in more detail: imagine a color scheme where window background is black, window text is white, view background is white view text is black.
you will get a white square on a white background for unchecked checkbox ...
I would really revert this part of the change

To be clear, that change hasn't been made.

You mean a white square on a black background? That could look odd if buttons aren't also normally white in that colorscheme, which might indicate that Button Background would be more semantically correct. By the way, the checkboxes already used View Background when selected unless they were drawn on top of a menu. In that case, they used Window Background when selected.

in more detail: imagine a color scheme where window background is black, window text is white, view background is white view text is black.
you will get a white square on a white background for unchecked checkbox ...
I would really revert this part of the change

To be clear, that change hasn't been made.

I'm confused: it has happened, no ? If I apply the patch I now get a white background (with normal breeze color scheme), behind checkboxes and radio button, always, which was not there before. This is , I think because of the change at line 3889.

You mean a white square on a black background?

No: if view color is white this would be white square (window text) on white background (view background).

That could look odd if buttons aren't also normally white in that colorscheme, which might indicate that Button Background would be more semantically correct. By the way, the checkboxes already used View Background when selected unless they were drawn on top of a menu.

No: in general background was not rendered. the View background was used for a special case of selection that happens only in views (if I remember the code right). With the standard breeze color scheme, a checkbox/radio button rendered in a window or a menu would never have a white background (from the view) drawn behind.
Unlike when applying the current patch.

In that case, they used Window Background when selected

What I meant is that I did not change the color of the checkbox background in this patch. I only made it so that the background would always be rendered.

It seems like it might be a good idea to detect whether or not the button is being rendered in a view area and then set it to View Background. Otherwise, it should be Window Background. Doing it that way would preserve the original look in most cases.

What I meant is that I did not change the color of the checkbox background in this patch. I only made it so that the background would always be rendered.

Clear enough. Indeed you did not change the colors, and it led to an unwanted visual change. Sorry for not having caught that up during review. (in fact it now leads to an inconsistency between checkboxes in menus and checkboxes in windows).

It seems like it might be a good idea to detect whether or not the button is being rendered in a view area and then set it to View Background.

yes

Otherwise, it should be Window Background. Doing it that way would preserve the original look in most cases.

This would lead to some regression (I think) for unchecked checkboxes in lists. (because of window background being used).