On Mon, May 27, 2013 at 04:49:01PM +0200, Andre Fischer wrote: > On 27.05.2013 15:16, Ariel Constenla-Haile wrote: > >On Mon, May 27, 2013 at 10:27:00AM +0200, Andre Fischer wrote: > >>Hi Ariel, > >> > >>I noticed a couple of commits that you made over the weekend. Only > >>one of these mentioned a bug id (121542, [1]). Some of the changes > >>introduce incompatible API changes, some but not all of which are > >>mentioned in the issue comments. > >that commit does not introduce an incompatible API change, on the > >contrary, is the (partial) revert of one: > >http://svn.apache.org/viewvc/openoffice/trunk/main/offapi/com/sun/star/awt/XPopupMenu.idl?revision=1413471&view=markup#l82 > >http://svn.apache.org/viewvc/openoffice/trunk/main/offapi/com/sun/star/awt/XPopupMenu.idl?revision=1425458&view=markup#l96 > > > >>Can you give us a short overview of what you are doing? > >As the commit message says, css::awt::XPopupMenu::execute() needs > >a css::awt::Rectangle. In most cases, only X and Y are needed to > >indicate the position where the PopupMenu will be executed, but in > >others a rectangle is needed, for example when executing a PopupMenu in > >a toolbar's drop-down click handler. > > That was the only one of r1486372, r1486373, r1486374, r1486375, > r1486377, r1486379, r1486380, r1486381, and r1486438 that belonged > to a bugzilla issue. The others do not. The individual commits > have messages that describe what they are doing. What is missing is > the "big picture". Some commits seem to belong together, some seem > to introduce new functionality that other developers might be > interested in. But there is no easy and short way to know.
No, there is no new functionality; it's just a clean-up of framework::PopupMenuController, that, as a UNO component, was not useful at all. The "big picture" is commit r1486377, all other related commits are preparation for that, or as a result of that: framework::PopupMenuController clean-up What the clean-up means is explained in the commit message. What might be useful for others is the reuse of PopupMenu filled at runtime by PopupMenuController's, this was already there by the original design, but it wasn't useful; now the concept is in use, as per the other commits, in the Open menu button on the StartCenter, the .uno:Open, .uno:AddDirect, and .uno:AutoPilotMenu toolbar items. Instead of copy & paste of code, they reuse the PopupMenuController's. This allowed removing the copy & paste (and killing the sfx2 implementation in favor of the new framework one). > Take commit r1486372 as example. Its message says "Some small > clean-up to use the PopupMenu ToolbarController" but the addition of > the ExecuteHdl_Impl callback seems to imply more than just cleanup. This was in the internal implementation, which was made protected, quite strange because it was only forward-declared (ToolboxController_Impl), hard to guess what was intended with this. I needed only access to some protected members, mainly the toolbar item ID, which was only available via a weird bool getToolboxId( sal_uInt16& rItemId, ToolBox** ppToolBox); so as I was there, I cleaned what was easy to clean; this class needs further clean-up - all related to m_bSupportVisiable is rather ugly, for example - but will be done as a mayor refactoring in the area, to make pure UNO ToolbarController's possible - right now they are impossible to implement because the toolbar item controller needs access to the vcl ToolBox to manipulate the item it controls. > Or commit r1486379 "Make .uno:Open a drop-down toolbar item". What > would that drop-down menu contain? See above. Both the Open menu button on the start center and the toolbar item have the same behaviour now. > Are there code changes in other > commits that belong to this? A bugzilla issue with a little more > information would be very nice. I didn't find a bug for the original framework::PopupMenuController commit http://hg.services.openoffice.org/OOO340/rev/4c837e6c327e so opening a new one just to state "I will clean you up" sounded too egocentric at that time. Regards -- Ariel Constenla-Haile La Plata, Argentina
pgp4FJVLu2mJW.pgp
Description: PGP signature
