Opened 2 years ago

Closed 2 years ago

#14900 closed defect (fixed)

Popup sample regression in popup panel size.

Reported by: AlecR Owned by:
Priority: low Milestone: 3.0.0
Component: GUI-all Version: stable-latest
Keywords: sizer regression Cc:
Blocked By: Blocking:
Patch: no

Description

I've just noticed a regression in this sample between 2.8.12 and 2.9.4. (Untested for earlier 2.9 versions.)

In 2.9.4, the ScrolledPopup does not display scrollbars; whilst in
2.8.12 it does for me. (The scroll and vscroll samples work for me in
2.9.4.)

I suspect that the regression is probably due to a sizer change, or perhaps the sample's code should be changed for 2.9.

Change History (10)

comment:1 Changed 2 years ago by pcor

  • Status changed from new to confirmed

comment:2 Changed 2 years ago by vadz

  • Component changed from base to GUI-all
  • Keywords regression added
  • Milestone changed from future to 3.0
  • Owner VZ? deleted
  • Status changed from confirmed to new

It would be great if somebody could look at this, I don't know when exactly did this break but it looks like a bad enough problem to merit being fixed before 3.0.

comment:3 Changed 2 years ago by ericj

I took a look at this, i think it wasn't broken, it was optimized.

wxScrollHelper::DoAdjustScrollbar() has code that hides the scrollbar if the client size is big enough for the whole content.

Even if you look at the old 2.8.x sample, you'll notice that the scroll area was pretty small, although the scrollbars were set to (1,1, 1000, 1000). Because the scrolled window is under control of a sizer, the sizer will re-set the virual size.

If you add " m_panel->SetAutoLayout(false); " to the end of the SimpleTransientPopup ctor, the scrollbars will be visible.

comment:4 Changed 2 years ago by AlecR

Thanks for looking at this, and your helpful analysis. Of course the end result is that there is currently no visible difference between the effects of pressing the two buttons; yet the user might reasonably expect to see a difference (as was shown in 2.8x). Since this is in a sample, I personally think that it would be preferable not to leave the situation as it is. Do you not agree? (I'd agree, tho', that this is a very low priority issue.)

comment:5 Changed 2 years ago by vadz

Yes, the thing that bothers me is that I don't see any relevant changes in the sample compared to 2.8. So why does it behave differently? I don't believe it's just a valid optimization neither because even with this:

  • samples/popup/popup.cpp

    diff --git a/samples/popup/popup.cpp b/samples/popup/popup.cpp
    index 4ab1ba6..d2a267e 100644
    a b void MyFrame::OnStartScrolledPopup(wxCommandEvent& event) 
    403403    wxLogMessage( wxT("================================================") ); 
    404404    delete m_scrolledPopup; 
    405405    m_scrolledPopup = new SimpleTransientPopup( this ); 
    406     m_scrolledPopup->GetChild()->SetScrollbars(1, 1, 1000, 1000); 
     406    m_scrolledPopup->GetChild()->SetScrollRate(10, 10); 
     407    m_scrolledPopup->GetChild()->SetVirtualSize(1000, 1000); 
    407408    wxWindow *btn = (wxWindow*) event.GetEventObject(); 
    408409    wxPoint pos = btn->ClientToScreen( wxPoint(0,0) ); 
    409410    wxSize sz = btn->GetSize(); 

the scrollbars are still not shown. So something is definitely wrong here, I just don't see what...

comment:6 follow-up: Changed 2 years ago by ericj

Calls to SetScrollRate(), SetVirtualSize() won't change anything as long as the window is in "autolayout" mode.

The following code in scrlwing.cpp will reset the virtual size:

void wxScrollHelperBase::HandleOnSize(wxSizeEvent& WXUNUSED(event))
{
    if ( m_targetWindow->GetAutoLayout() )
    {
        wxSize size = m_targetWindow->GetBestVirtualSize();

        // This will call ::Layout() and ::AdjustScrollbars()
        m_win->SetVirtualSize( size );
    }
    else
    {
        AdjustScrollbars();
    }
}

Adding " m_panel->SetAutoLayout(false); " to the end of the SimpleTransientPopup ctor disables autolayout and the scrollbars appear.

Like said before, the sample didn't even work correctly in 2.8.x, but as "some" scrollbars appeared, it probably wasn't noticed until now. The only difference is that 2.9.x now hides the scrollbars if they aren't needed.

Whether it's considered a bug that the sizer overrides the virtual size the client requested, is a different issue.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 2 years ago by vadz

  • Status changed from new to confirmed

Replying to ericj:

Calls to SetScrollRate(), SetVirtualSize() won't change anything as long as the window is in "autolayout" mode.

Ah, yes, of course, sorry for missing this the first time you explained it.

Like said before, the sample didn't even work correctly in 2.8.x, but as "some" scrollbars appeared, it probably wasn't noticed until now. The only difference is that 2.9.x now hides the scrollbars if they aren't needed.

I'd still like to understand what exactly is the change here but it's likely to be correct/beneficial so the main motivation to find it would be to document it in docs/changes.txt. For now I'll just change the sample to show scrollbars.

Whether it's considered a bug that the sizer overrides the virtual size the client requested, is a different issue.

I think it makes sense for it to work like this, if you're using sizers you shouldn't set the size manually. But it might need to be documented too...

Thanks again for the explanations!

comment:8 in reply to: ↑ 7 Changed 2 years ago by ericj

Replying to vadz:

I'd still like to understand what exactly is the change here but it's likely to be correct/beneficial so the main motivation to find it would be to document it in docs/changes.txt.

The magic happens in wxScrollHelper::AdjustScrollbars() in scrlwing.cpp. Apparently that method was refactored several times. Now, in 2.9.x it calls a new method wxScrollHelper::DoAdjustScrollbar() which hides the scrollbar if the client size is big enough to fit the virtual size. That code did not exist in 2.8.x.

comment:9 Changed 2 years ago by vadz

Thanks for your debugging once again!

AFAIR this change wasn't intentional... But OTOH it does seem to make sense. Still strange that it didn't behave like this in 2.8 though, I'm pretty sure that scrolled windows didn't show scrollbars there if they were not needed in general. So could it be that this only didn't happen when they were managed by sizers?

comment:10 Changed 2 years ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [73319]) Ensure that scrollbars are shown in scrolled popup in the popup sample.

Don't call SetScrollbars(), this is overridden by sizer logic later. Instead,
add a sufficiently big window to the popup and make fix the size of the popup
itself to be smaller to ensure that the scrollbars do show.

Closes #14900.

Note: See TracTickets for help on using tickets.