Bug 97665 - New icon theme isn't applied to never opened menus
Summary: New icon theme isn't applied to never opened menus
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (show other bugs)
Version:
(earliest affected)
5.2.0.0.alpha0+
Hardware: All Linux (All)
: medium normal
Assignee: Maxim Monastirsky
URL:
Whiteboard: target:5.2.0
Keywords:
: 89515 97085 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-02-08 21:32 UTC by Maxim Monastirsky
Modified: 2017-01-12 19:14 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maxim Monastirsky 2016-02-08 21:32:55 UTC
Steps to reproduce:

1. Make sure you're using the gtk vclplug + VCL menus (not Unity global menu).
2. Open Writer.
3. Tools->Options.., Change icon theme.
4. Open any menu that you never opened before (e.g. Insert).

The menu shows icon from the old icon theme (but next time you open that menu, it will finally pick the right icons).
Comment 1 Maxim Monastirsky 2016-02-08 21:50:27 UTC
Most likely a regression of:

commit eda624641b34a7d4315388c8ec1aebe44f63982e
Author: Bjoern Michaelsen <bjoern.michaelsen@canonical.com>
Date:   Sun May 18 14:28:39 2014 +0200

    lp#1296715: refresh invalidated menus

Surprisingly the root cause of this bug is exactly the same as for lp#1296715 (which the above commit just workarounded):

framework::MenuBarManager updates the menu contents in the activate callback, which has a code like this:

762         if ( m_bActive )
763             return false;
764 
765         m_bActive = true;

and MenuBarManager::Deactivate has m_bActive = false;

This means that a second call to Activate, without Deactivate first will do nothing - so the menu contents won't be updated on the next activation.

This isn't an issue for a "real" menu activation made by the user, because Deactivate will be called as soon as the mouse pointer leaves that menu. The problem arises with the "fake" activation made by the SalMenu to create the menu structure - with the current code in framework it must do explicit Deactivate after each Activate. This isn't done right now.
Comment 2 Maxim Monastirsky 2016-02-08 22:18:01 UTC
Possible solutions:

1. Not run any Unity code when native menus aren't actually used. (Sounds like a good idea anyway.)

2. Kill the "if ( m_bActive ) return false;" (m_bActive itself should be retained - see its usage in MenuBarManager::SetItemContainer) - Can it cause any harm? I'm not sure why this check is there in the first place - so what I'm missing?

3. Add "pMenuBar->HandleMenuDeActivateEvent(mpVCLMenu);" to GtkSalMenu::ActivateAllSubmenus.

Doing any of the last two _alone_ fixes lp#1296715 even with reverting the original commit completely. No need to update the whole (!) menu structure on any slot status change, or to leave active status listener in framework. Another benefit is to fix (the very annoying) Bug 89515 - which caused by the late activation introduced by lp#1296715.

(Actually partially reverting also the lp#1085169 commit - so that we'll update only one menu at a time seems to cause no harm. Maybe lp#1085169 was also related to m_bActive - who knows? Anyway I can't reproduce it now.)
Comment 3 Maxim Monastirsky 2016-02-08 22:25:19 UTC
One problem I noticed is with the HUD. We do need to update the whole menu structure when it's activated, but for some reason the hud_activated callback is never called. So in my tests I had to retain the status listener in framework + adding this thing to the end of MenuBarManager::statusChanged:

        if ( m_bHasMenuBar && !m_bActive )
            m_pVCLMenu->UpdateNativeMenu();

where Menu::UpdateNativeMenu is a wrapper I created to call GtkSalMenu::Update().
Comment 4 Maxim Monastirsky 2016-02-08 22:31:38 UTC
@Björn: Hi, Any thoughts on this? What's the preferred solution? Have I missed something? Any idea why the HUD callback doesn't work? Thanks in advance.
Comment 5 Commit Notification 2016-02-15 12:09:50 UTC
Maxim Monastirsky committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2abdcfd641883f246fe78f2fbe38499c9382c059

tdf#97665 Let's hope that over activation isn't really needed

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Maxim Monastirsky 2016-02-15 12:11:16 UTC
Please let me know if there will be any regressions because of this.
Comment 7 Maxim Monastirsky 2016-02-15 12:12:10 UTC
*** Bug 89515 has been marked as a duplicate of this bug. ***
Comment 8 David H. Gutteridge 2016-03-05 21:18:15 UTC
This fix has broader implications, as it addresses an issue in prior versions of LibreOffice where there are significant menu updating lags that cause the list of open documents in the "Window" menu to get out of sync. (At least, if one is using the GTK2 VCL under Gnome 3.) The downstream bug I filed for Fedora has more detail in this regard: https://bugzilla.redhat.com/show_bug.cgi?id=1313559
Comment 9 Maxim Monastirsky 2017-01-12 19:14:50 UTC
*** Bug 97085 has been marked as a duplicate of this bug. ***