#15381 closed defect (fixed)

Ribbonbar (even though collapsed) becomes expanded when it repaints

Reported by: snowleopard2 Owned by:
Priority: normal Milestone:
Component: GUI-all Version: 2.9.5
Keywords: ribbonbar ribbon Cc: rakeshthp, wxBen
Blocked By: Blocking:
Patch: yes

Description

Run the Ribbon example and either double click a tab on it or click the toggle button in the right corner, this will make the ribbon collapse. Now, either click in the text area in the program or give another program the focus and go back to the ribbon demo--note how the ribbon is now expanded again. Basically, when you collapse the ribbon, anything that triggers a paint event will make it expanded again.

Digging through the code, I think a call to HideIfExpanded() from OnKillFocus() is the culprit. When the demo loses the focus and my ribbon is collapsed, I step into this function and "m_ribbon_state" is set to "wxRIBBON_BAR_MINIMIZED"--which makes sense. However, in this function it expects "m_ribbon_state == wxRIBBON_BAR_EXPANDED", otherwise it will set it to "wxRIBBON_BAR_PINNED" and then it gets expanded because of that later. If I had to guess, the first line of HideIfExpanded() should be this:

m_ribbon_state == wxRIBBON_BAR_MINIMIZED || m_ribbon_state == wxRIBBON_BAR_EXPANDED

and NOT this:

m_ribbon_state == wxRIBBON_BAR_EXPANDED

but I'm not 100% on what the intention is in this function--the ribbon bar is supposed to hide itself when it loses the focus? At any rate, this seems wrong, because it's really annoying how it keeps expanding itself whenever I click in the program's client area or switch between programs.

This worked OK in 2.9.4 (there you could double click the tab to hide it), this seems to be a new problem in 2.9.5. Thanks.

Attachments (1)

update.diff download (465 bytes) - added by snowleopard2 14 months ago.
Patch for fix

Download all attachments as: .zip

Change History (6)

Changed 14 months ago by snowleopard2

Patch for fix

comment:1 Changed 14 months ago by snowleopard2

  • Patch set

After applying this patch, it seems to work just like the ribbons in Windows 8. I see what the "pin" behavior is now, and it works as expected too.

comment:2 Changed 14 months ago by vadz

  • Cc rakeshthp wxBen added

This doesn't seem very logical to me: if it's already minimized, why should we hide it? But I don't really know this code, it would be great if some of the people who worked on this code recently could look at this (adding them to cc).

comment:3 Changed 13 months ago by rakeshthp

Hi,

Though this doesn't seem to be logical,

"Basically, when you collapse the ribbon, anything that triggers a paint event will make it expanded again"

explains it more. The purpose of HideIfExpanded() is to hide the ribbon if it is maximized. The if part is to hide the ribbon and else part is to show the ribbon. This makes sense when the application is active and has focus set to itself. Now consider the situation when the ribbon state is "wxRRIBBON_BAR_MINIMIZED". i.e. the ribbon is minimized. When the focus is lost, HideIfExpanded() is invoked where (with previous code), the else part was executing, and therefore with the "wxRIBBON_BAR_MINIMIZED" state also the ribbon was expanding.

I agree with the current piece of patch and I strongly feel it should be committed. Of course, after rigorous testing on all the platforms.

Thanks & regards

Rakesh Patil

comment:4 Changed 13 months ago by vadz

Thanks for the explanation! I think I see it now but I am still not sure why should we hide the panels if it's already minimized. IOW, shouldn't we just do nothing if it's minimized? I also still have some trouble understanding the behaviour in the pinned case: why should we show the panels when losing focus when the ribbon is pinned? Maybe I just don't understand what does "pinned" mean? Looking at the code this seems more like the normal ribbon state...

Based on my current understanding, my candidate patch would be

  • src/ribbon/bar.cpp

    diff --git a/src/ribbon/bar.cpp b/src/ribbon/bar.cpp
    index 8edae51..1bad16a 100644
    a b void wxRibbonBar::HitTestRibbonButton(const wxRect& rect, const wxPoint& positio 
    12691269 
    12701270void wxRibbonBar::HideIfExpanded() 
    12711271{ 
    1272     if ( m_ribbon_state == wxRIBBON_BAR_EXPANDED ) 
     1272    switch ( m_ribbon_state ) 
    12731273    { 
    1274         HidePanels(); 
    1275         m_ribbon_state = wxRIBBON_BAR_MINIMIZED; 
    1276     } 
    1277     else 
    1278     { 
    1279         ShowPanels(); 
    1280         m_ribbon_state = wxRIBBON_BAR_PINNED; 
     1274        case wxRIBBON_BAR_EXPANDED: 
     1275            m_ribbon_state = wxRIBBON_BAR_MINIMIZED; 
     1276            // Fall through 
     1277 
     1278        case wxRIBBON_BAR_MINIMIZED: 
     1279            HidePanels(); 
     1280            break; 
     1281 
     1282        case wxRIBBON_BAR_PINNED: 
     1283            ShowPanels(); 
     1284            break; 
    12811285    } 
    12821286} 
    12831287 

where I also changed the code to use a switch to ensure the compiler warns us if we forget to add new enum elements to it if any are added later (which seems to have happened here).

What do you think?

comment:5 Changed 12 months ago by VZ

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

(In [74953]) Fix unwanted ribbon expansion on focus loss.

The ribbon unexpectedly showed itself on focus loss when it was minimized.

Fix this and also use switch statement on m_ribbon_state to ensure that the
compiler warns us if we forget to add the code for handling any new elements
of wxRibbonDisplayMode enum.

Closes #15381.

Note: See TracTickets for help on using tickets.