Winforms Tree widget

[Oliver-Leigh]

Welcome to the Tree widget for the Windows platform!

How it works:

The Tree widget is based on the Table widget, except with some extra structure:

• The display data is managed by a new class called StateTree which is a wrapper for the TreeSource. It keeps track of which nodes are expanded/collapsed and then makes/modifies a display list which is sent to the UI
• The nodes are expanded/contracted by clicking a state-change arrow on the left of the row. The state-change arrow also highlights when the cursor is hovering over it. These functionalities are achieved through mouse event actions and a hit-test which determines whether the event has occurred in an appropriate area.
• The non-leaf nodes are custom painted by handling the NM_CUSTOMDRAW message using the subclass method from Icon support for all columns in Winforms Table #4164.

Notes on Styling:

The styling is based on that of ListView groups, with some modifications. Unfortunately the combination of virtual mode and groups is not supported in WinForms, and is undocumented for Win32. Also, looking at the expansion arrow in the picture on the left below, I don't think that this style is being actively updated.

The solution is to simply custom draw the row. Here is the current iteration of the Tree widget:

• I've put the state-change arrows on the left to align with the macOS and GTK implementations.
• The colors used by the native ListView groups are undocumented (as far as I know) so I've used the system color hotlight which is a close match.
• Since the whole row is custom painted, any of the styling can be changed!

Notes for testing:

One of the hardest parts was to get the selection to play nicely with the state-change clicks. The interactions between the clicks-hit-tests and selection is somewhat complicated. Make note of any weirdness you observe in this area!

Changes to the Table widget

I've made some minor changes to the WinForms Table widget code. This is mainly tidying things up a little, but also I've modified some methods so they can be more easily shared with the Tree widget.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

This is awesome - thanks! I've flagged a couple of minor style things; but on the whole, the implementation looks really solid (to the extent that I understand win32 stuff), and it definitely works in all my testing.

The one issue of substance that I noticed is around the style of the Tree, and the "blue line" rendering of tree nodes. One detail that isn't exercised in the tree example app is that tree nodes can have data. In the tree example app, you could give a title to each decade:

The app is currently using an empty string for each "non column 1" field. That's probably a change we should make to the example, so that it's obvious - but with that change, in the Windows rendering, as soon as a leaf becomes a node, we lose the text for the rest of the node.

You've mentioned that the row style is customisable - is that sort of change in the realm of the possible? I'm guessing that might mean we lose the blue bar - I'm not sure if that's a "dealer's choice" thing from you, or if it's idiomatic on Windows.

[Oliver-Leigh]

The app is currently using an empty string for each "non column 1" field. That's probably a change we should make to the example, so that it's obvious - but with that change, in the Windows rendering, as soon as a leaf becomes a node, we lose the text for the rest of the node.

Oh I completely missed that! Also for the example, it might be nice to have some higher depth nodes as well.

You've mentioned that the row style is customisable - is that sort of change in the realm of the possible? I'm guessing that might mean we lose the blue bar - I'm not sure if that's a "dealer's choice" thing from you, or if it's idiomatic on Windows.

We can have the extra columns and have blue bars between the non-empty columns (if that's something we want?). I'll have a think about the best way to do it and make the changes!

[freakboy3742]

The app is currently using an empty string for each "non column 1" field. That's probably a change we should make to the example, so that it's obvious - but with that change, in the Windows rendering, as soon as a leaf becomes a node, we lose the text for the rest of the node.

Oh I completely missed that! Also for the example, it might be nice to have some higher depth nodes as well.

Agreed - maybe adding a 1900s and 2000s grandparent nodes? If we need to add some 2010s data to make that separation make sense...

We can have the extra columns and have blue bars between the non-empty columns (if that's something we want?). I'll have a think about the best way to do it and make the changes!

I guess my question is the extent to which the blue bar is idiomatic. If it is, then yes, retaining it makes sense; if it's not, then I wonder if it's more trouble than it's worth, and we should stick to just the expansion arrow.

[Oliver-Leigh]

Agreed - maybe adding a 1900s and 2000s grandparent nodes? If we need to add some 2010s data to make that separation make sense...

Good idea! I can look into it after this is finished.

I guess my question is the extent to which the blue bar is idiomatic. If it is, then yes, retaining it makes sense; if it's not, then I wonder if it's more trouble than it's worth, and we should stick to just the expansion arrow.

Good question. I think you'd have a hard time arguing that either the blue or the blue bar is idiomatic of Windows as a whole, simply because the style is always changing. I've decided to remove both of these. I can always put them back in if there's popular demand!

[Oliver-Leigh]

One extra thing to note is that I've disabled the focus rectangle for now. Getting it to work properly was becoming very complicated. I'll have to think a bit more about how to do it properly.

[freakboy3742]

I needed to make a small fix for compatibility with a recent change in how widgets are registered (which was the source of the test failure); I also needed to test the "text in node columns" thing, so I've taken the opportunity to update the tree example to have multiple layers of depth and text in the non-leaf columns.

Otherwise, this is incredible - thanks for filling a really notable gap in the Winforms backend!

Looks like there's a test coverage issue.

[freakboy3742]

Looks like there are some test coverage issues.

We had a similar issue with the Qt backend - there's some edge cases that need to be caught for error handling completeness purposes, but they either should never happen, or they can't be reproduced reliably under testbed conditions.

We don't have a problem adding # pragma: no cover to the code if there are cases that can't be covered - but they should always be accompanied with a comments backing up why a no cover is valid under the circumstances.

It may also be necessary to add additional test cases; if that's the case, feel free to add extra steps to an existing testbed case, or an entirely new testbed case, as required.

[Oliver-Leigh]

I needed to make a small fix for compatibility with a recent change in how widgets are registered (which was the source of the test failure);

Thanks! I thought it would be something like this, but I didn't want to make a mess.

Looks like there are some test coverage issues.

We had a similar issue with the Qt backend - there's some edge cases that need to be caught for error handling completeness purposes, but they either should never happen, or they can't be reproduced reliably under testbed conditions.

We don't have a problem adding # pragma: no cover to the code if there are cases that can't be covered - but they should always be accompanied with a comments backing up why a no cover is valid under the circumstances.

It may also be necessary to add additional test cases; if that's the case, feel free to add extra steps to an existing testbed case, or an entirely new testbed case, as required.

I've fixed the coverage issues now. Most of them were because of the various mouse event handlers that are being used.

When writing these tests I found an interesting discrepancy between the platforms: On macOS and Windows, when a selected item is hidden, it is deselected. However this is not the case on Linux. I'm not sure what the desired behavior is. I based the behavior of the Windows version on the macOS version, so those two align at least.

[freakboy3742]

Nice work getting to 100% coverage; however, the approach you've taken needs a little bit of tweaking.

As a general rule, we avoid using direct if platform == ... type checks in testbed. If something needs to be tested, it should be tested everywhere. That then leads to three possible outcomes:

1. The test behavior is different on different platforms, and that's an acceptable "platform behaviour" discrepancy
2. The test behavior is different on different platforms, which is an indication of a problem
3. The test hasn't historically been needed on other platforms, and requires a new probe implementation on all those other platforms.

Most of the tests that you've added here fall into case 1 or 2 - as you've identified, there is inconsistent behavior between platforms. The question is: "does that matter?".

My immediate reaction is "probably not?". I'm willing to be convinced otherwise, but for me, this feels a bit like platform-specific behavior; as long as the selection is never wrong, I don't think the specific behavior around what is selected matters that much.

On that basis, I'd be happy with modifications to these tests that allow them to run on every platform, asserting as much of the behaviour that is consistent, and commenting where the behaviour isn't. Essentially - take our all the "if platform" conditions that you've added here; and when a test fails, comment out that specific assertion, adding a comment above it noting that the behavior is inconsistent between platforms. That means the code is exercised, and we assert everything that is consistent.

The only test that doesn't match this is the test_mouse_events test. This is a case where I can see there's a path of testing needed for Windows - the same tests could be implemented for other platforms, but it evidently isn't needed for coverage. There's two possible paths here:

1. We take one of the test assertions that is invoked early in the test (e.g., assert_item_mouse_hover, and implement it on every other backend as a pytest.skip(). This means the test runs, and we could implement the test for those backends, but it isn't a hard-requirement for the testbed to pass on that platform
2. We add the ability to define and run "platform specific tests" to the testbed runner. This has come up previously (in the context of Qt's tree tests, coincidentally). I think this is worth pursuing, but it's also a much bigger piece of work.

So - my inclination for this one is to take path (1).

[freakboy3742]

I can see what you're doing here, but we don't use this pattern in testbed. Details in the overall review comments.

[Oliver-Leigh]

I've cleaned up the tests as you suggested. In the end there is the test_mouse_events test which I use pytest.skip for the other platforms, and two instances of exercising but not asserting:

• Expand/contract a non-visible node.
• Contract a node whose child is selected.

Also the focused item rectangle has been implemented! It's a bit more complicated than I'd like, but that's due to some shortcomings shared between Win32 and WinForms

[freakboy3742]

Awesome - thanks for those fixes; this looks great!

[johnzhou721]

One extra thing to note is that I've disabled the focus rectangle for now. Getting it to work properly was becoming very complicated. I'll have to think a bit more about how to do it properly.

For the record: Focus rectangles on Windows is not idiomatic anymore; on modern WinUI apps there's no distinction between "selected" and "focussed". So this is actually a good thing overall.