Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks - this is impressive stuff, and the complex bits are all well documented.
I've flagged a couple of minor things that stood out from a review. The only other thing that stood out is refresh handling. We should have some mechanism for initiating a refresh. Two easy approaches would be:
- A Qt-style "header bar" that has a refresh button; or
- Adding a "refresh" option to the context menu.
I don't have strong feelings one way or the other on that one. Longer term, I think we're going to need to reconsider how refresh works; but for now, pragmatism definitely wins.
Regarding the mouse message issue - I'm not aware of any issues with UAC eating message, but I can't rule that out. The other possibility (and it's one that we've seen a lot in the past) is processing speed - it's not uncommon for tests to fail because events are being processed slightly faster or slower under certain testing circumstances. Looking at the select/deselect tests in particular, they're failing right after a "redraw"; adding a small delay to that redraw might help, as might an explicit "wait until selection" loop (i.e., putting the testbed into a "while selection not updated" loop with a longer timeout).
If that doesn't help, I'm willing to be pragmatic; if we're able to test most of the widget, with a couple of small gaps (for some measure of small), I can probably live with that - although it would not be my preferred option. I'd need to see specifics to make a call on how "small" that coverage gap can be.
| if not probe.supports_deselect: | ||
| pytest.skip("The probe for this backend doesn't support deselection.") |
There was a problem hiding this comment.
This works; however, when there's a clear method we can call that wraps the "unsupported" behavior, we usually wrap the skip logic into that method. In this case, adding a deselect_all() method to all backends and raising the skip/xfail in that method means one less probe attribute; plus we can differentiate between "is not implemented on this platform yet" and "cannot be implemented on this platform"
There was a problem hiding this comment.
This is fixed now. I went through the Android code again, and the solution blindingly obvious.
| # According to the MicroSoft documentation, an application must call | ||
| # InitCommonControlsEx must before creating a common control. | ||
| self._init_common_controls_ex = cc32_cls.INITCOMMONCONTROLSEX() | ||
| self._init_common_controls_ex.dwSize = sizeof(cc32_cls.INITCOMMONCONTROLSEX) | ||
| self._init_common_controls_ex.dwICC = wc.ICC_LISTVIEW_CLASSES | ||
| InitCommonControlsEx(byref(self._init_common_controls_ex)) |
There was a problem hiding this comment.
Is this something we should be doing as part of the initialization of the app, rather than the widget?
Does it need to be retained as a property of the widget?
There was a problem hiding this comment.
Is this something we should be doing as part of the initialization of the app, rather than the widget?
It doesn't matter where it's called, as long as it's called before the control in question is created. Also, if doesn't matter if this is called multiple times, and the effect is cumulative for different inputs.
Since this particular widget is the only one that needs this to be called, I think it's best to leave it here.
Does it need to be retained as a property of the widget?
No, good catch!
There was a problem hiding this comment.
I think we should clarify the comment a bit, since it's not exactly easy to find that this is specific to ICC_LISTVIEW_CLASSES. "An application must call InitCommonControlsEx for each type of common controls one wants to create before creating an instance of it" may work better as a comment.
I should have added a note for this! I've pretty much sorted out how to do a macOS style refresh. The dynamics are somewhat fiddly, so I think it's best to get the revamp PR nailed down and then to have a separate PR for the refresh. I also like the idea of a refresh option in the context menu (in addition to the scroll refresh). It will be easy to implement and might add to the overall usability since the scroll refresh is a bit of a hidden feature. What do you think?
These are some good suggestions. Its worth getting to the bottom of this! I'll look into this a bit more and maybe send through some debug commits too. |
I'm OK with "pull-to-refresh" being a later feature; but we need something in place right now. Putting it in the context menu is an easy solution, so we might as well go with that. We can decide later whether it's a permanent thing, an optional thing, or something we remove when pull-to-refresh is an option. |
Oliver-Leigh
force-pushed
the
winforms-detailed-list-revamp
branch
from
25d3c19 to
a02059f
Compare
3 participants
A revamp of the DetailedList widget for Windows
The revamp of the DetailList widget has two key advantages over the existing implementation:
How it works:
The widget is based on the Win32 List-View UI using the tile view option. The reason for using a Win32 approach here is that the WinForms List-View UI does not support tile view in virtual mode.
The tile view natively places the tiles into rows, so here the tiles are resized so that there is only one column. Unfortunately, there is a bug in tile view where the tile sizes can become "stuck" upon too many resizes. Much of the code here is to overcome cascading issues resulting from this.
The approach taken to overcome the resize bug is to completely custom-draw the tiles.
The tiles and icons automatically resize themselves based on the size for the font used. Since the fonts are resized on DPI changes, the widget will adjust itself to DPI changes.
The actions are implemented using a context menu à la macOS. A Win32 approach is used for the context as well. This is because the style of the WinForms context menu has not been updated for recent version of Windows (see, for example, dotnet/winforms#2476).
A note about the tests
I needed to add a deselect test to get full code coverage. My current setup can't run the new tests and I was unable to write this test for Android. So the tests will most likely need some fixing/adjustments.
It seems that the online Windows testbed is blocking the mouse button Windows messages. I've tried a few work arounds, but I think it might be some UAC settings that are blocking them. Maybe this is easily fixed? Otherwise I can rework the tests and add in some extra praga: no cover statements.
PR Checklist: