wxDataViewCtrl tree rendering is broken when expanding/collapsing nodes on macOS (Issue #25162)
Jon Evans
Description
On macOS, a dataview rendered as a tree control has broken rendering while it is running the animation for expanding/collapsing nodes.
In the sample app, the behavior appears as the nodes rearranging themselves / other node's content appearing in a node's position.
This can be seen in the dataview sample app:
https://github.com/user-attachments/assets/ffcc7656-e1c0-43f7-9661-ac17c5daf418
Platform and version information
- wxWidgets version you use: 3.2.6
- wxWidgets port you use: wxOSX
- OS and its version: macOS 14.7.2
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I do see a lot of brokenness on the variable height page, but not on any of the other ones. Does KiCad use wxDV_VARIABLE_LINE_HEIGHT and/or do you see problems on the other pages too?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
No, we do not use wxDV_VARIABLE_LINE_HEIGHT. I only see the problem on that page, though. I am not sure what the connection is; at first I thought this must be a KiCad bug because I just checked the first page of the sample, then I found that on the variable line height page it has the issue.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
BTW, purely from looking at the code, I guess the problem is that when using wxDV_VARIABLE_LINE_HEIGHT we change the renderer value here
and this overwrites the actual value stored there by outlineView:objectValueForTableColumn: byItem:, so perhaps we could fix this by simply remembering the original value and restoring it after calling SetValue() and GetSize().
But I don't really know this code at all. FWIW it seems to date from e86edab (Add wxDataViewCtrl implementation for OSX/Cocoa (closes #10617: wxDataView for wxOSX-Cocoa), 2009-05-08), but I think it could have been written by @hwiesmann and not Robert himself, so pinging him just in case he has any suggestions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
No, we do not use
wxDV_VARIABLE_LINE_HEIGHT. I only see the problem on that page, though. I am not sure what the connection is; at first I thought this must be a KiCad bug because I just checked the first page of the sample, then I found that on the variable line height page it has the issue.
Yes, but it's still a complete mystery to me why would it happen if you don't use wxDV_VARIABLE_LINE_HEIGHT (or, rather, why would it happen only for you and not in the sample).
Perhaps it somehow gets enabled accidentally, e.g. you use some other style which can't be used with wxDVC and just happens to have the same value (0x20)? Grepping the sources I see that wxBK_BOTTOM, wxLC_REPORT and wxRIGHT (as well as a few others which seems even less likely, e.g. wxTE_MULTILINE) have this value, could you have somehow used them (which is possible because, unfortunately, flags-based API is completely type-unsafe, of course...)?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
I have discovered that the KiCad bug is a KiCad bug (but haven't figured out how to solve it yet). The dataview sample does indeed have similar symptoms but can't have the same cause.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
(the gist of the bug is that we use a wxDataViewCustomRenderer instead of a normal text renderer, and it appears as though ::Render is called during the expand/collapse animation without first ::SetValue being called on the renderer, so it is rendering with some other cell's value.
The call stack in question:
LIB_TREE_RENDERER::Render(wxRect, wxDC *, int) lib_tree_model_adapter.cpp:103
wxDataViewCustomRendererBase::WXCallRender(wxRect, wxDC *, int) datavcmn.cpp:1058
-[wxCustomCell drawWithFrame:inView:] dataview.mm:1100
-[NSWrapperCellView drawRect:] 0x000000018c7fada8
_NSViewDrawRect 0x000000018bd388cc
-[NSView _recursive:displayRectIgnoringOpacity:inContext:stopAtLayerBackedViews:] 0x000000018c6cafc8
-[NSView(NSLayerKitGlue) _drawViewBackingLayer:inContext:drawingHandler:] 0x000000018bd382d0
-[NSViewBackingLayer drawInContext:] 0x000000018c26d52c
It seems like maybe drawWithFrame should first be setting the renderer's value before calling WXCallRender or am I misunderstanding something?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25162)The trouble is that if we need to call SetValue() here, I have no idea how to do it becayse we don't have neither the item nor the column that we need to retrieve this value from the model. I hope there is some NSOutlineView function being called first...
If I understand you correctly, I should be able to reproduce the bug in the sample just by using a custom renderer for the first column in the first page, right? If so, I'll try to do it and debug this — unless you have some better ideas and/or prefer to do it yourself.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
craftyjon left a comment (wxWidgets/wxWidgets#25162)Yes, by changing MyFrame::BuildDataViewCtrl in the sample to use MyCustomRenderer for the first column in the first tab, I can make the problem start happening.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
craftyjon left a comment (wxWidgets/wxWidgets#25162)After the delegate returns, the table view sets only the alpha and frame properties, and then only when animating rows as they slide in or out.
We see this issue when the animation is running. Perhaps there is a mismatch here between the NSOutlineView assumptions and the wxDataViewCtrl abstraction: it looks like maybe the Apple API assumes that each cell will have its own custom renderer that will preserve state between calls to SetValue, while wxWidgets seems to assume that each column can share a single custom renderer and that that one custom renderer can keep having its value changed to represent different cells in the column.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25162)The question is whether outlineView:willDisplayCell:forTableColumn:item: is called before displaying every item or not? I hope it is, and we just do something wrong in it, because otherwise I don't see how could this be made to work...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
craftyjon left a comment (wxWidgets/wxWidgets#25162)No, I confirmed with tracing that willDisplayCell is not called when animating the expand/collapse, just drawWithFrame. So it seems to assume that once the NSCell is created, its renderer maintains state.
I don't see how could this be made to work...
If I am right about how NSCell is intended to work, it seems like wxCustomCell needs one renderer per cell (or another way of maintaining state internally)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25162)If I am right about how NSCell is intended to work, it seems like
wxCustomCellneeds one renderer per cell (or another way of maintaining state internally)
But this would require an O(N) number of renderers, where N is the number of items in the control. I can't believe there is no way to avoid it...
Besides, things seem to work correctly for wxDataViewTextRenderer, but it also sets the cell value from its MacRender(), called from outlineView:willDisplayCell:forTableColumn:item:.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
craftyjon left a comment (wxWidgets/wxWidgets#25162)One interesting thing:
in wxDataViewCustomRenderer::MacRender, GetValue().GetString() always returns an empty string.
in wxDataViewTextRenderer::MacRender, GetValue().GetString() always returns the string from the model.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
craftyjon left a comment (wxWidgets/wxWidgets#25162)tempelmann/XojoListboxTV#1 not a lot of info there but kind of describes the same problem and implies that they solved it by not re-using cells
I have spent about as much time on this as I care to for now, I am burnt out with the lack of documentation on Apple's old APIs. It seems like the cell-based outline view is basically deprecated? Maybe the view-based replacement doesn't have this problem
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Thomas Tempelmann
tempelmann left a comment (wxWidgets/wxWidgets#25162)tempelmann/XojoListboxTV#1 not a lot of info there but kind of describes the same problem and implies that they solved it by not re-using cells
Right. You must not re-use cell views, but instead use one per cell. But also avoid leaks.
Typically, for NSView based tableviews, you'd invoke viewForItem:inView:forTableColumn: to get a cell view, i.e. the TableView manages a set of such views for you (it keeps a pool that's just about as many as are currently needed to show in the scroll view's visible area).
If you're using a Cell based tableview, and want to provide custom cells, though, then I don't know right now how to do that. I believe I used to, but forgot.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25162)Thanks Thomas for confirming that this is indeed as bad as I thought.
I guess the only way forward is to do what you suggest, i.e. create and keep renderers for each cells and then clean them up when they are not used any longer (but how can we even be sure about this?).
@craftyjon How bad/urgent is this for you? This is a big change and I almost surely won't be able to do it before 3.3.2 unless we delay it for even longer.
Of course, any contributions from others would be welcome.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jon Evans
craftyjon left a comment (wxWidgets/wxWidgets#25162)@vadz this is not urgent, don't delay a release over it. It's a visual glitch that makes things seem janky but it does not impair functionality.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Thomas Tempelmann
tempelmann left a comment (wxWidgets/wxWidgets#25162)@vadz Not sure how your renderers look like, but all you need to keep around is the object that "hold" the cell's view(s).
Also, no worries about lifetime. Basically, they're only used during the short time a refresh on the TableView is performed. Once a new refresh is happening, you'll get asked once more to provide these views for each cell.
And since everything happens on the main thread, all you need to do is, once you're asked to return these objects, you start a timer with zero-delay, or use dispatch_async on the main thread, which will then be invoked once the current redraw handling is done.
Not sure about animated actions here, though, so maybe either only start the time once your custom cell objects get a "draw" call, or delay cleanup by one second. Also make sure that your code considers the case where new redraws keep happening because the user is constantly scrolling: In that case, only dispose of the older objects returned initially but not everything, i.e. do not use a single pool, but one per "redraw session", so that you don't accidentally free objects that were just created by the latest redraw during a scroll, and have not been actually used yet.
In other words, do a lazy disposal of the objects, maybe even simply time out every object after a second, disposing of it then if it hasn't been used again (which would be the case if you get another redraw request for the same object).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.