Fix wxGetBestTreeSize function (PR #25843)
ssrlive
This PR will close #25839 .
It also includes the invisible child nodes of the tree control in the best estimate width calculation.
The demo file is here hello.zip
And the fixed screenshot is here.
image.png (view on web)You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25843
Commit Summary
- 2a76018 Fix wxGetBestTreeSize function
File Changes
(1 file)
- M src/common/treebase.cpp (33)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25843.patch
- https://github.com/wxWidgets/wxWidgets/pull/25843.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 6316ccb Fix wxGetBestTreeSize function
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25843)To compute the size including the hidden items we probably could use GetIndent()*depth + GetTextExtent().
I also wonder if we could just expand the tree in wxTreebook before computing its best size and then collapse it to show only the top level items?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)I think we can add a parameter to the constructor of wxTreeCtrl to indicate whether to expand the nodes during initialization.
—
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#25843)I think we can add a parameter to the constructor of
wxTreeCtrlto indicate whether to expand the nodes during initialization.
No, why should it be a ctor parameter? It's enough to call Expand() recursively after constructing it.
I think whether the child nodes in the tree are expanded or not will not affect the current implementation of
wxGetBestTreeSize.
It should, because I guess that GetBoundingRect() doesn't return the correct rectangle for collapsed items, but if they're expanded, it really should work.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)I agree.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)Why do so many tests fail?
—
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#25843)Why do so many tests fail?
If you click on the links, you will see the error messages.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)I don't know the reason.
make[1]: Leaving directory '/__w/wxWidgets/wxWidgets/utils/wxrc'
make: Target 'all' not remade because of errors.
Error: Process completed with exit code 2.
—
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#25843)Please search for error above. E.g. use search box on GitHub to search for *** or error etc.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)I don't think these failures are caused by my changes. I'd appreciate if you could check the build script.
—
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#25843)I don't think these failures are caused by my changes. I'd appreciate if you could check the build script.
Sorry but you're wrong. I haven't looked at all of the errors, but I had looked at some of them before commenting and there are plenty of them due to missing header in the code you added. You really should look at the build logs carefully, this is not exactly rocket science.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- d8b275e Fix wxGetBestTreeSize function
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for fixing the problems, but there are still a few things that could be improved here.
Most problematic is, of course, the hardcoded 20px value. Did you only test this under Windows? And, if so, at which DPI (100%, 200%)?
> @@ -215,6 +230,23 @@ wxGetBestTreeSize(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id, wxSize& size)
size.IncTo(wxSize(rect.GetRight(), rect.GetBottom()));
}
+ else if (id != treeCtrl->GetRootItem())
+ {
+ // Estimate width for collapsed (invisible) node
+ wxString label = treeCtrl->GetItemText(id);
+ wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
+ dc.SetFont(treeCtrl->GetFont());
+ wxSize textSize = dc.GetTextExtent(label);
+ int indent = treeCtrl->GetIndent();
+ int iconWidth = 0, h = 0;
+ int imageIndex = treeCtrl->GetItemImage(id);
+ if (treeCtrl->GetImageList() && imageIndex != -1)
+ treeCtrl->GetImageList()->GetSize(imageIndex, iconWidth, h);
+ int depth = wxGetTreeItemDepth(treeCtrl, id);
+ int extraPadding = 20; // experience-based value, can be adjusted
This should at least use treeCtrl->FromDIP() but it really shouldn't be necessary in the first place. Can we add an extra indent instead, perhaps?
> @@ -215,6 +230,23 @@ wxGetBestTreeSize(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id, wxSize& size)
size.IncTo(wxSize(rect.GetRight(), rect.GetBottom()));
}
+ else if (id != treeCtrl->GetRootItem())
+ {
+ // Estimate width for collapsed (invisible) node
+ wxString label = treeCtrl->GetItemText(id);
+ wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
+ dc.SetFont(treeCtrl->GetFont());
+ wxSize textSize = dc.GetTextExtent(label);
+ int indent = treeCtrl->GetIndent();
+ int iconWidth = 0, h = 0;
+ int imageIndex = treeCtrl->GetItemImage(id);
+ if (treeCtrl->GetImageList() && imageIndex != -1)
⬇️ Suggested change
- if (treeCtrl->GetImageList() && imageIndex != -1) + if ( treeCtrl->HasImages() && imageIndex != wxTreeCtrl::NO_IMAGE )
> @@ -215,6 +230,23 @@ wxGetBestTreeSize(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id, wxSize& size)
size.IncTo(wxSize(rect.GetRight(), rect.GetBottom()));
}
+ else if (id != treeCtrl->GetRootItem())
+ {
+ // Estimate width for collapsed (invisible) node
+ wxString label = treeCtrl->GetItemText(id);
+ wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
We should avoid using wxClientDC in the new code.
- wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl)); + wxInfoDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
> @@ -215,6 +230,23 @@ wxGetBestTreeSize(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id, wxSize& size)
size.IncTo(wxSize(rect.GetRight(), rect.GetBottom()));
}
+ else if (id != treeCtrl->GetRootItem())
+ {
+ // Estimate width for collapsed (invisible) node
+ wxString label = treeCtrl->GetItemText(id);
+ wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
+ dc.SetFont(treeCtrl->GetFont());
This is unnecessary, the DC already inherits window's font.
⬇️ Suggested change- dc.SetFont(treeCtrl->GetFont());
> @@ -215,6 +230,23 @@ wxGetBestTreeSize(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id, wxSize& size)
size.IncTo(wxSize(rect.GetRight(), rect.GetBottom()));
}
+ else if (id != treeCtrl->GetRootItem())
+ {
+ // Estimate width for collapsed (invisible) node
+ wxString label = treeCtrl->GetItemText(id);
+ wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
+ dc.SetFont(treeCtrl->GetFont());
+ wxSize textSize = dc.GetTextExtent(label);
+ int indent = treeCtrl->GetIndent();
+ int iconWidth = 0, h = 0;
+ int imageIndex = treeCtrl->GetItemImage(id);
+ if (treeCtrl->GetImageList() && imageIndex != -1)
+ treeCtrl->GetImageList()->GetSize(imageIndex, iconWidth, h);
⬇️ Suggested change
- treeCtrl->GetImageList()->GetSize(imageIndex, iconWidth, h); + iconWidth = treeCtrl->GetImageLogicalSize(treeCtrl, imageIndex).x;
> @@ -200,6 +202,19 @@ void wxTreeCtrlBase::SetItemState(const wxTreeItemId& item, int state)
DoSetItemState(item, state);
}
+// Helper to get depth of a tree node
+static int wxGetTreeItemDepth(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id)
Use of this function results in O(N^2) algorithm which is probably not catastrophic for wxTreebook but still seems quite unnecessary: just add int depth = 0 parameter to wxGetBestTreeSize() and call it with depth + 1 when calling it recursively, then you don't have to compute the depth for each item at all.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 8103d93 Fix issue in Linux, since GetBoundingRect always return true
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)After patch 8103d93 , the behave in Linux looks fine.
image.png (view on web)—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- e0b1d6d Fix issue in macOS
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)After this patch, it looks fine in macOS. e0b1d6d
image.png (view on web)—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 239d84b wxGetTreeItemDepth removed
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive commented on this pull request.
> @@ -200,6 +202,19 @@ void wxTreeCtrlBase::SetItemState(const wxTreeItemId& item, int state)
DoSetItemState(item, state);
}
+// Helper to get depth of a tree node
+static int wxGetTreeItemDepth(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id)
Finished in 239d84b
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev commented on this pull request.
> {
wxRect rect;
- if ( treeCtrl->GetBoundingRect(id, rect, true /* just the item */) )
+ if ( treeCtrl->GetBoundingRect(id, rect, true /* just the item */)
+ && rect.width > 0 && rect.height > 0 ) // check for valid rect
⬇️ Suggested change
- && rect.width > 0 && rect.height > 0 ) // check for valid rect + && !rect.IsEmpty() ) // check for valid rect
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 43773cb Update src/common/treebase.cpp
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 2ded700 GetImageLogicalSize usage
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive commented on this pull request.
> @@ -215,6 +230,23 @@ wxGetBestTreeSize(const wxTreeCtrlBase* treeCtrl, wxTreeItemId id, wxSize& size)
size.IncTo(wxSize(rect.GetRight(), rect.GetBottom()));
}
+ else if (id != treeCtrl->GetRootItem())
+ {
+ // Estimate width for collapsed (invisible) node
+ wxString label = treeCtrl->GetItemText(id);
+ wxClientDC dc(const_cast<wxTreeCtrlBase*>(treeCtrl));
+ dc.SetFont(treeCtrl->GetFont());
+ wxSize textSize = dc.GetTextExtent(label);
+ int indent = treeCtrl->GetIndent();
+ int iconWidth = 0, h = 0;
+ int imageIndex = treeCtrl->GetItemImage(id);
+ if (treeCtrl->GetImageList() && imageIndex != -1)
+ treeCtrl->GetImageList()->GetSize(imageIndex, iconWidth, h);
Apply in 2ded700
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 39e4ef5 Apply some suggestion
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, looks good now but I'm still bothered by this hard-coded 20. Have you tried using extra indent instead?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)not okay in windows.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)—
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#25843)What about this idea: get the rect of the visible item (there must be at least one), then compute its estimated size using your method and then compute the width (and maybe even height?) difference. And then add this difference to all the sizes computed using your method.
AFAICS this should work reliably in all cases.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)to use indent directly. Anyway, there should be at least one indent on the
left side of any node, and adding an indent on the right side of the
longest entry is a good reason for aesthetic reasons.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)—
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#25843)There is clearly too much space in your Mac screenshot, so I think it would be worth making the code a bit more complex if this would be more precise. And I don't think it's that complicated anyhow, it's enough to just extract the code for computing the item extents into a separate function and call it twice.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- 705f246 Use indent to replace extraPadding
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)Here's what I've done.
I've replaced extraPadding with indent.
If you don't like it, feel free to change it accordingly.
You can make changes directly on my fork, commit them to my branch, and then merge them into the main wxWidgets branch.
https://github.com/ssrlive/wxWidgets/blob/fix-tree-ctrl/src/common/treebase.cpp#L234
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
@ssrlive pushed 1 commit.
- c6d9248 make extraPadding different in difference platforms
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
ssrlive
ssrlive left a comment (wxWidgets/wxWidgets#25843)After the patch c6d9248
It looks the same behaviors.
image.png (view on web) image.png (view on web) image.png (view on web)—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.