Fix (not) showing values for empty cells in wxDVC using wxDataViewVirtualListModel in wxGTK (PR #22409)
VZ
This should fix #22359 and, AFAICS, doesn't have any negative effects.
I couldn't find any real explanation about why was this check added in the first place, but there doesn't seem to be any performance reason to not make it even for the virtual list models: it is called for every item in the model, which makes it totally unsuitable for big numbers of items, but this was already the case before anyhow and the extra overhead of calling HasValue() for it shouldn't be noticeable compared to calling GetValue(). But please let me know if I'm missing anything here or, also, if there are any other situations in which this still doesn't fix the original problem.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22409
Commit Summary
- 1c9c48c Don't show value-less wxDataViewVirtualListModel cells in wxGTK
- 610eeb4 Inline wxGtkTreeSetVisibleProp() function
File Changes
(1 file)
- M src/gtk/dataview.cpp (30)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22409.patch
- https://github.com/wxWidgets/wxWidgets/pull/22409.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
@vslavik approved this pull request.
Thanks! This looks good to me w.r.t. correctness.
Could it be further simplified by removing the HasValue() check and if statement, as PrepareForItem() already does that viaCheckedGetValue()? I'm not saying it should be done, but checking if my understanding is correct and this condition is just an optimization.
I think that as it is now (and this logic is shared by some other ports), there's a very subtle, mostly academical difference between !HasValue() and not returning a value from GetValue():
- If
GetValue()doesn't return a value,PrepareForItem()still enables/disables cell state even for empty cells - apparently intentionally. - But if
HasValue()returns false,PrepareForItem()is skipped altogether and enabled state is not adjusted.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 8aefedc Remove duplicated HasValue() call from wxGTK wxDataViewCtrl code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Yes, I think you're right, thanks, it looks better to simplify this even if I don't think this can make any difference in practice because SetEnabled() is only used by the custom renderers AFAICS and their Render() shouldn't be called anyhow if the cell is invisible. Done now.
BTW, concerning the "apparently intentionally" part, I think 361c635 (Use wxDataViewRenderer::PrepareForItem() in all ports, 2015-08-29) just prevented the existing behaviour (but added an explicit comment about it), so I'm not totally certain if this is needed neither, but I'm definitely not going to change it without a good reason -- and this call is cheap, so it shouldn't result in any performance problems neither.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #22409 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.