search

beeware / toga

A Python native, OS native GUI toolkit.
https://toga.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
4.3k stars 668 forks source link

Fixed divider widget on WinForms #2667

Closed proneon267 closed 3 months ago

proneon267 commented 3 months ago

Derived from #2484:

I have corrected the WinForms Divider implementation to use a WinForms.Panel instead of a WinForms.Label, as WinForms.Panel is more suitable for a Divider widget and allows setting a background color.

The previous implementation did not produce any visual results when background color was set. This is because it was producing the Divider widget by squeezing the borders of the WinForms.Label and setting its height to 2 px, which hid its background, resulting in just the borders being visible.

The reason why it was passing tests was because it was setting the background color of the WinForms.Label. However, the probe only checks through the native APIs, which returned the set background color. Since its background was hidden, there were no visual results.

PR Checklist:

proneon267 commented 3 months ago

I'm also not convinced that Panel is necessarily a better widget. Label was picked as an implementation because the "divider" is using the 3D rendering style of the border, with no content, as the widget. That's why the width/height is 2 - it's 1 pixel each for the top/bottom or left/right side of the divider, providing a vaguely 3d embossed "line". Switching to Panel means it's a solid 1px line. This might give more flexibility in setting the background color... but I'm not entirely convinced that's even desirable. If you've got evidence that a flat style is more consistent with Winforms style, then make your case; but given that other Winforms buttons have various 3D effects, I'm inclined to say dividers should, too.

Here are 2 images with the two different implementations: Image 1(Original Hack Implementation) Image 2(Proposed Implementation)
Screenshot 2024-06-19 032416 image

The textbox widget is included to compare its style with the default WinForms widget rendering on Windows 11.

The divider in image 2 looks much closer to the style of the underline in the textbox widget. Thus, ensuring consistent look across widgets.

The only 3D effect that becomes visible is when the divider height is increased: image And at that point, the divider looks much like an empty box rather than a divider.

Further, as noted before, the background color when set on the divider is not visible.

Image 1(Original Hack Implementation) Image 2(Proposed Implementation)
image image

It's not at all clear what you mean by "behaves correctly" - the PR description mentions some side effects of the existing implementation, but nothing that I would interpret as "behavior" of the widget; and there's no new tests validating any new behavior.

By behaving correctly, I meant that the proposed implementation now makes the background color visible(as on other backends), and that it now looks like a divider instead of an empty box when the divider has some thickness.

However, I do agree that I have mistakenly put "allows changing of dimensions" in the changelog. I will change it.

And, again, there's no tests for this new capability. The widget didn't historically have background color tests largely because no other platform (except perhaps web?) allows users to change the color of dividers; if you're proposing that background colors can/should be honoured, then adding tests for this is the bare minimum.

I would have enabled background color tests on the divider widget, but that would have caused the same issues as identified in #2478, which then led to #2484 .

freakboy3742 commented 3 months ago

Here are 2 images with the two different implementations: ... The divider in image 2 looks much closer to the style of the underline in the textbox widget. Thus, ensuring consistent look across widgets.

Why is the underline of a textbox widget a good point of comparison? I'm looking for more of a general style guide, than a direct comparison to one specific feature of one specific widget.

As a counterexample: If you have a toolbar, and add a separator bar to that toolbar, the separator has a "shadow":

Screenshot 2024-06-20 at 12 32 37 PM

The only 3D effect that becomes visible is when the divider height is increased:

The divider height shouldn't ever be increased. A divider is a thin line, not a block. The fact that the cross-axis size of the divider can be changed at all is a bug.

Further, as noted before, the background color when set on the divider is not visible.

Is that a bug? I could make an argument that the color of a divider shouldn't be configurable. Frankly, I'd make that same claim for most color changes - I don't understand the need people have to change the colors of default system widgets - but that's a much bigger battle.

This is why the testbed divider tests don't have background color tests at present - color change isn't a supported feature for divider on macOS; and on GTK, it's not exposed as a simple option.

It's not at all clear what you mean by "behaves correctly" - the PR description mentions some side effects of the existing implementation, but nothing that I would interpret as "behavior" of the widget; and there's no new tests validating any new behavior.

By behaving correctly, I meant that the proposed implementation now makes the background color visible(as on other backends), and that it now looks like a divider instead of an empty box when the divider has some thickness.

In which case, the changenote should refer to the specific fix(es) that have been made, not "behaves correctly".

And, again, there's no tests for this new capability. The widget didn't historically have background color tests largely because no other platform (except perhaps web?) allows users to change the color of dividers; if you're proposing that background colors can/should be honoured, then adding tests for this is the bare minimum.

I would have enabled background color tests on the divider widget, but that would have caused the same issues as identified in #2478, which then led to #2484 .

If the change was just the switch from Label to Panel, then probe change you've got here would be sufficient testing. But you can't make a claim that some other behaviour now "works", and then not provide any tests of that. Outside of a very limited set of edge cases where testability is an actual concern, tests aren't an optional part of a PR - and those cases need to be individually negotiated.

In this case, if the feature can't be tested until the background transparency issue is addressed, then that needs to be solved first. Or, you land #2478 without supporting background color changes, and then fix background colors for all widget transparency testing works on winforms.

proneon267 commented 3 months ago

The rendering of WinForms widgets on Windows 11 is a bit flat, as Microsoft's new widget design is also flatter.

However, on a bigger note, I agree with you about not exposing color customization for divider widget. This also highlights that I have left out a bigger and important part of the work while doing #2484 i.e., prepare a list of widgets on the backends that I have changed and discuss with you, if they should be able to change their background color or not.

So, this PR will be closed, and I will prepare the list of widgets and discuss them with you on #2484.