Corrected wxTreeCtrl::DoGetBestSize(), added DoGetBestClientWidth (PR #26097)
RobertRoeb
For the generic wxTreeCtrl:
- Corrected ::DoGetBestSize(), added DoGetBestClientWidth
- Adapted both to the same calculation of the required virtual size that is used in AdjustScrollbars() removing the need for adding magic 4
- Now, importantly, wxTreeBook actually calls a real DoGetBestClientWidth() based on a known height and can correctly figure out if a vertical scrollbar is needed
- This removes the ugly horizontal scrollbar in wxTreeBook when a the horizontal scrollbar appears.
- Tested on macOS 26
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26097
Commit Summary
- 178a27f Corrected wxGenericTreeCtrl::DoGetBestSize(), added DoGetBestClientWidth()
File Changes
(2 files)
- M include/wx/generic/treectlg.h (3)
- M src/generic/treectlg.cpp (52)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26097.patch
- https://github.com/wxWidgets/wxWidgets/pull/26097.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#26097)This image illustrates the problem: when all branches are expanded, a horizontal scrollbar appears because the current code miscalculated the width. This looks broken and you cannot even add some manual extra space.
Treebook.Tahoe.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.
RobertRoeb
@RobertRoeb pushed 1 commit.
- 542f13a Whitespaces removed
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Thanks but, again, it looks like there is an "obvious" (unless I'm missing something, which is perfectly possible, but please explain what to me then) better way of doing this by defining DoGetBestClientSize() and reusing it from the new function.
> + // add the border + const wxSize& borderSize = GetWindowBorderSize(); + size.x += borderSize.x; + size.y += borderSize.y;
Could we avoid this by overriding DoGetBestClientSize() instead of DoGetBestSize()?
And if not, this should be written simply as
size += GetWindowBorderSize();
> + // make sure all positions are calculated as normally this only done during + // idle time but we need them for base class DoGetBestSize() to return the + // correct result + wxConstCast(this, wxGenericTreeCtrl)->CalculatePositions(); + + wxSize size = wxTreeCtrlBase::DoGetBestSize(); + + // DoGetBestClientWidth calculates if a vertical scrollbar is + // needed based on the given height + + // Use the calculation from AdjustMyScrollbars() + size.y += PIXELS_PER_UNIT+2; // one more scrollbar unit + 2 pixels + size.x += PIXELS_PER_UNIT+2; // one more scrollbar unit + 2 pixels + size.x = (size.x / PIXELS_PER_UNIT) * PIXELS_PER_UNIT; + size.y = (size.y / PIXELS_PER_UNIT) * PIXELS_PER_UNIT;
This is just an exact copy of DoGetBestClientSize(), right? Why not simply call it from here?
> - // and the border has to be rounded up to a multiple of PIXELS_PER_UNIT or - // scrollbars still appear - const wxSize& borderSize = GetWindowBorderSize(); + // Use the calculation from AdjustMyScrollbars()
Could we, perhaps, add a function like wxSize GetTotalAreaFromClientArea(wxSize) and use it both here and in AdjustMyScrollbars()?
> + size.x = (size.x / PIXELS_PER_UNIT) * PIXELS_PER_UNIT; + size.y = (size.y / PIXELS_PER_UNIT) * PIXELS_PER_UNIT;
This rounds up, unlike AdjustMyScrollbars() which rounds down. A comment explaining why this is the right thing to do here but not there would be quite helpful because this doesn't seem obvious to me.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> + size.x = (size.x / PIXELS_PER_UNIT) * PIXELS_PER_UNIT; + size.y = (size.y / PIXELS_PER_UNIT) * PIXELS_PER_UNIT;
I think I am using the same calculations, so I must be overlooking something. Please point me to it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
> + // add the border + const wxSize& borderSize = GetWindowBorderSize(); + size.x += borderSize.x; + size.y += borderSize.y;
So maybe for background, this is the sequence of calls that I am trying to fix:
a) wxBookCtrlBase::DoSize() wants to lay out the controller (tree control on the left) and the page (right)
b) it calls wxBookCtrlBase::GetControllerSize() to determine the size of the controller
c) that calls m_bookctrl->GetBestWidth(sizeClient.y); to allow the controller to figure out how much it has to stretch out horizontally
d) wxWindowBase::GetBestWidth(int height) calls DoGetBestClientWidth(height);
Therefore, I had to override wxGenericTreeCtrl::DoGetBestClientWidth(height) to make it properly report the width of the tree depending on whether or not it has a vertical scrollbar. This function needs to know what is essentially the primary direction of expansion, in our case the tree should expand horizontally to not have a horizontal scrollbar and should use scrolling vertically (if it does not fit, obviously).
DoGetBestSize() calculates the size so that (or as if) no scrollbar are present in either direction, that is a different task.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + size.x = (size.x / PIXELS_PER_UNIT) * PIXELS_PER_UNIT; + size.y = (size.y / PIXELS_PER_UNIT) * PIXELS_PER_UNIT;
Sorry, it looks like I misread the code, either here or there, because rereading it now I don't see the difference. But this just (completely inadvertently) proves my point about having GetTotalAreaFromClientArea(): if we called it from both places, it would be perfectly obvious that the calculation is the same.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> + // add the border + const wxSize& borderSize = GetWindowBorderSize(); + size.x += borderSize.x; + size.y += borderSize.y;
I understand that it's not enough to override only DoGetBestSize(). The comment above only said that we could override DoGetBestClientSize() instead of it to avoid having to add borders manually.
Overridden version of DoGetBestClientWidth() would still remain (but hopefully reuse this one instead of duplicating it).
—
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#26097)Should I make the changes discussed above myself or is it not desirable to make them for some reason I'm missing?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb pushed 1 commit.
- 33dd0a9 Factor code for better re-use
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> + // make sure all positions are calculated as normally this only done during + // idle time but we need them for base class DoGetBestSize() to return the + // correct result + wxConstCast(this, wxGenericTreeCtrl)->CalculatePositions(); + + wxSize size = wxTreeCtrlBase::DoGetBestSize(); + + // DoGetBestClientWidth calculates if a vertical scrollbar is + // needed based on the given height + + // Use the calculation from AdjustMyScrollbars() + size.y += PIXELS_PER_UNIT+2; // one more scrollbar unit + 2 pixels + size.x += PIXELS_PER_UNIT+2; // one more scrollbar unit + 2 pixels + size.x = (size.x / PIXELS_PER_UNIT) * PIXELS_PER_UNIT; + size.y = (size.y / PIXELS_PER_UNIT) * PIXELS_PER_UNIT;
There was no DoGetBestClientSize() so this question puzzled me for a while. I just added it and made DoGetBestSize() and DoGetClientBestWidth() use it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
> - // and the border has to be rounded up to a multiple of PIXELS_PER_UNIT or - // scrollbars still appear - const wxSize& borderSize = GetWindowBorderSize(); + // Use the calculation from AdjustMyScrollbars()
I added this as a local function since it has no general use
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.