Fix best size computation in wxGenericTreeCtrl (PR #27043)
VZ
Implement DoGetBestClientWidth() so that the control can compute the appropriate width when its height is known, as is the case when it's used as wxTreeBook controller: this prevents the appearance of ugly and unnecessary horizontal scrollbar in this case.
Add a unit test checking that the best size is computed correctly.
Closes #26097.
This is my version of Robert's original change including a unit test. @RobertRoeb could you please test this under Mac?
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/27043
Commit Summary
- 6184ebd Fix best size computation in wxGenericTreeCtrl
File Changes
(3 files)
- M include/wx/generic/treectlg.h (3)
- M src/generic/treectlg.cpp (52)
- M tests/controls/treectrltest.cpp (64)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/27043.patch
- https://github.com/wxWidgets/wxWidgets/pull/27043.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 5 commits.
- 86415be Fix returning wrong value of wxGenericListCtrl::GetCountPerPage()
- e50c794 Extract wxGetScrollbarSpacing() function from wxGTK wxWindow
- 417785f Fix GetScrollbarSize() in wxGTK when scrollbar-spacing is non-0
- f387863 Use GetScrollbarSize() instead of wxSYS_HSCROLL_[XY]
- 15df498 Fix best size computation in wxGenericTreeCtrl
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)I am happy to test them once they are in master. I don't have time before.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27043)Sorry, I'm puzzled by this, what do you mean? It takes the same amount of time to test this branch or master, so will you test it when you do have time?
Just for the record, to test the branch you need to
$ git fetch g...@github.com:vadz/wxWidgets.git generic-treectrl-getbestclientwidth $ git checkout FETCH_HEAD $ ... rebuild, test ... $ git checkout master
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27043)Anyhow, I guess it's useless to wait for this to be tested, so I'll merge it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)Hi there, I tested the code in master with the test below - and maybe we don't agree on the "right" behaviour. I think that the size of the tree area should be such that no horizontal scrollbar appears even when a vertical one appears (since the window is not high enough, obviously). With the code in master, the look is good if there is no vertical scrollbar. But if you make the window less high and expand the tree, then both scrollbars will appear. I think, the algorithm of wxTreebook should aim to achieve that the horizontal scrollbar never appears and I think my patch did it. That is the one issue that is avoided with overlay scrollbars.
Image: TwoScrollbars (view on web)// Purpose: test wxTreebook layout
#include "wx/wxprec.h"
#ifndef WX_PRECOMP
#include "wx/wx.h"
#endif
#include "wx/treebook.h"
class MyApp : public wxApp
{
public:
virtual bool OnInit() wxOVERRIDE;
};
class MyFrame : public wxFrame
{
public:
MyFrame(const wxString& title);
void OnQuit(wxCommandEvent& event);
private:
wxDECLARE_EVENT_TABLE();
};
wxBEGIN_EVENT_TABLE(MyFrame, wxFrame)
EVT_MENU(wxID_EXIT, MyFrame::OnQuit)
wxEND_EVENT_TABLE()
wxIMPLEMENT_APP(MyApp);
bool MyApp::OnInit()
{
if ( !wxApp::OnInit() )
return false;
MyFrame *frame = new MyFrame("wxTreebook App");
frame->Show(true);
return true;
}
MyFrame::MyFrame(const wxString& title)
: wxFrame(NULL, wxID_ANY, title, wxDefaultPosition, wxSize( 400, 200 ))
{
auto menubar = new wxMenuBar();
auto filemenu = new wxMenu();
filemenu->Append( wxID_EXIT, "Quit" );
menubar->Append( filemenu, "File" );
SetMenuBar( menubar );
auto treebook = new wxTreebook( this, -1 );
wxPanel *panel;
for (int i = 0; i < 4; i++)
{
panel = new wxPanel( treebook, -1 );
panel->SetBackgroundColour( *wxYELLOW );
treebook->AddPage( panel, "Main chapter" );
panel = new wxPanel( treebook, -1 );
panel->SetBackgroundColour( *wxGREEN );
treebook->AddSubPage( panel, "Short subchapter 1" );
panel = new wxPanel( treebook, -1 );
panel->SetBackgroundColour( *wxBLUE );
treebook->AddSubPage( panel, "Short subchapter 2" );
panel = new wxPanel( treebook, -1 );
panel->SetBackgroundColour( *wxWHITE );
treebook->AddSubPage( panel, "Long Long Long Long Long subchapter 3");
}
}
void MyFrame::OnQuit(wxCommandEvent& WXUNUSED(event))
{
Close(true);
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27043)This must be a macOS bug... The overlay scrollbar is not supposed to decrease the client size and I thought this did work in wxOSX because the test should be checking it, but apparently it doesn't.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)Sorry, I used the wrong grammar: overlay scrollbars WOULD solve the issue, but wxMac does not have them (or rather only on iPhone after my change from last year)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)So I just looked at my old patch again, and the problem in Master has not changed. The code in
wxSize wxBookCtrlBase::GetControllerSize() const
calls
m_bookctrl->GetBestWidth(sizeClient.y);
which only works if the control has a specific
int wxGenericTreeCtrl::DoGetBestClientWidth(int height) const
that takes into account the extra width of the vertical scrollbar. So my patch added that missing method and added:
if (height < size.y)
{
// Add space for vertical scrollbar
return size.x + wxSystemSettings::GetMetric( wxSYS_VSCROLL_X );
}
Without the code, you will get two scrollbars. I don't understand why you would not get two scrollbars on wxMSW (as there are no overlay scrollbars there either, I think). Or maybe GetClientSize() on wxMSW always add space for scrollbars? How does my little sample look on MSW? Maybe I will test it myself.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27043)I'm getting more and more lost here... Why doesn't wxOSX use overlay scrollbars? I thought all scrollbars under macOS were overlay.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27043)FWIW your example works correctly for me (== no horizontal scrollbar shown) with GTK_OVERLAY_SCROLLING=0 under Linux.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)I just checked my mini sample on Win11 and it always gives extra space for the vertical scrollbar, even if it is not visible. That is using the native wxTreeCtrl, I assume. We can simulate that in the generic control by always adding the space for the vertical scrollbar in DoGetBestSize
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)I think on macOS, we use distinct scrollbar widgets. If the native NSScrollView was used, we would have overlay scrollbars, I assume. I just tested on macOS again. wxGetSystemSettings reports a width of 16 pixels, but you actually have to add 20pixels to make the horizontal scrollbar disappear
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)I don't understand how it can work with non-overlay scrollbars on GTK+. Where is the space added for the scrollbar? GetBorderSize()? GetClientSize()? GetBestSize()?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#27043)Answering myself: wxGTK does it in wxWindowGTK::DoGetClientSize() so maybe we should do it there in wxMac as well?
Or should we switch to using NSScrollView? That will probably break some code...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.