Bug 95938 - Toolbar context menu has no commands
Summary: Toolbar context menu has no commands
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
5.1.0.0.alpha1
Hardware: All All
: high major
Assignee: Samuel Mehrbrodt (allotropia)
URL:
Whiteboard: target:5.1.0
Keywords: regression
Depends on:
Blocks:
 
Reported: 2015-11-19 22:21 UTC by Maxim Monastirsky
Modified: 2016-10-25 19:19 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 2015-11-19 22:21:14 UTC
As the title says, the toolbar context menu, as well as the overflow menu have no commands.

This is a regression of:

commit 27bdc70d83d4a4f1ebb89429f61b39084e739aaa
Author: Samuel Mehrbrodt <Samuel.Mehrbrodt@cib.de>
Date:   Wed Nov 18 13:28:46 2015 +0100

    Toolbar: Refactor insert toolitem code
    
    Change-Id: Icb615164c4dc3e96048829805a3eb0faa7d88e4e

We're now using ToolBox::InsertItem(const OUString& ... to insert toolbar buttons from ToolBarManager, and it sets new item ids as (30000 + items count). The problem is that ToolBox::UpdateCustomMenu sets the menu ids as (item id + 57344) which overflows the 16-bit int, and breaks a whole bunch of code that relies on it.

Not sure what's the best way of fixing this. We could, for instance, start items from 1 instead of 30000 (0 is reserved for separators and should not be used). Hard to believe this will create any trouble. This 30000 is also used in vcl/source/window/builder.cxx:1671 for items without a command, but in reality a ToolBox will never have > 30000 items, so it would never clash.

On the other hand, maybe it's better to change TOOLBOX_MENUITEM_START from 57344 to a lower value?

@Samuel: What do you think on this? What's the best solution?
Comment 1 Samuel Mehrbrodt (allotropia) 2015-11-20 06:51:10 UTC
Hi Maxim,

many thanks for reporting & investigating!
I proposed a patch, and I did both options you suggested (reduce TOOLBOX_MENUITEM_START and remove this 30000 constant).
I guess it's save to have the ItemID be "GetItemCount() + 1", or am I missing something?

https://gerrit.libreoffice.org/#/c/20072
Comment 2 Maxim Monastirsky 2015-11-20 07:57:57 UTC
(In reply to Samuel Mehrbrodt from comment #1)
> I guess it's save to have the ItemID be "GetItemCount() + 1", or am I
> missing something?
Yes, it seems so. But might be a good idea to ask Caolán/Kendy first, as they introduced those 30000.
Comment 3 Commit Notification 2015-11-20 09:05:31 UTC
Samuel Mehrbrodt committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95938 Toolbar context menu has no commands

It will be available in 5.1.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 4 Samuel Mehrbrodt (allotropia) 2015-11-20 09:07:41 UTC
(In reply to Maxim Monastirsky from comment #2)

> But might be a good idea to ask Caolán/Kendy first, as they introduced those 30000.

Pushed now. Kendy, Caolán, if there are any objections against that patch, let me know. Can then still prepare a follow-up fix.