Disabling commands as described here is not working
http://wiki.openoffice.org/wiki/Documentation/DevGuide/WritingUNO/Disable_Commands

Ariel, can you describe which commands are disabled and what the expected outcome is?

Rev 1461104:
Added navigator panel for Impress.

Rev 1463515:
Added navigator panel for Writer.

Hiding the button for showing/hiding (called ZoomIn/ZoomOut in the navigator source code) all controls below the top two rows of icons.
While it might be possible to do this (the existing code access the parent window, believing that it is a child window; an assumption that is not true in the sidebar) this would just waste the space, as the navigator is the only panel in the deck.

Please ignore my last two comments, they where supposed for issue 121960.

Created attachment 80507 [details]
Commands.xcu

This is the Commands.xcu file inside the extension on attachment 80464 [details]

The commands that are disabled by configuration are:

.uno:Bold
.uno:Italic
.uno:Underline
.uno:FontColor
.uno:BackgroundColor
.uno:CharFontName
.uno:FontHeight
.uno:ParagraphDialog
.uno:PageDialog

Best way to see the effect:

- Create a new Writer document
- dt + F3 to insert dummy text
- select a word and set it in bold, select another one and set it in italic
- open the Extension Manager and install the extension on attachment 80464 [details], trying to keep both windows visible (the Writer document and the Extension Manager), looking at the formatting toolbar

When installing the extension finished, you'll see that the disabled commands are indeed disabled on the formatting toolbar, the status is updated right after the extension is installed. If you open a new window, you'll see that the disabled commands are removed from the toolbar (this is because the toolbar and menu managers remove disabled command on UI element initialization).

Disabled commands cannot be executed by selecting the respective toolbar/menu item, nor by the keyword shortcut (for example, Ctrl + B does not set a word in bold if .uno:Bold is disabled).

There are two bugs in the sidebar:

a) feature status is not reflected, the respective items are enabled in the sidebar though disabled by configuration

b) the respective slot is executed regardless of being disabled

A third bug, may not be easy to implement:

c) disabled items are not removed

(a) is due to the use of a (class derived from) SfxControllerItem to track status updates, this bypasses the dispatch framework (by "dispatch framework" I mean all the UNO-based layer mainly implemented in the new framework module: the framework's Frame, as dispatch provider, takes care of returning an empty dispatch when the command is disabled, see Frame::queryDispatch() in main/framework/source/services/frame.cxx, and Desktop::queryDispatch() in main/framework/source/services/desktop.cxx).
Side note: the sfx2 context menu implementation has a similar bug, because the SfxMenuControl is derived from SfxControllerItem: with .uno:Bold disabled, right-click on a bold word, the context menu will display that Style - Bold is checked; though selecting the menu item to remove the bold does not work, in this case bug (b) does not apply.

(b) is due to the way the slot is executed through the SfxDispatcher and not through the SfxBindings, the latter forwards calls to the dispatch framework, the former bypasses it. An example, for the Bold in the text properties: 

main/svx/source/sidebar/text/TextPropertyPanel.cxx
IMPL_LINK(TextPropertyPanel, ToolboxFontSelectHandler, ToolBox*, pToolBox)

mpBindings->GetDispatcher()->Execute(SID_ATTR_CHAR_WEIGHT, SFX_CALLMODE_RECORD, &aWeightItem, 0L);

(c) makes the sidebar behave different from the toolbar and menubar, where disabled items are removed (opengrok SvtCommandOptions in the framework module).

Dispatch interception, as explained here http://wiki.openoffice.org/wiki/Documentation/DevGuide/OfficeDev/Dispatch_Interception, is also broken.

I'm not sure if opening a separated bug, because the underlying reasons are the same.

Thanks for the detailed bug description and the analysis of the root cause.  I will look into this as soon as the integration into trunk is done.

(In reply to comment #7)
> Dispatch interception, as explained here
> http://wiki.openoffice.org/wiki/Documentation/DevGuide/OfficeDev/
> Dispatch_Interception, is also broken.
> 
> I'm not sure if opening a separated bug, because the underlying reasons are
> the same.

I'll open a new bug, now that the branch has been rebased to trunk I can reuse a dispatch interceptor component I had (but needed 4.0 to compile with the SDK), I'll attach the example in that bug.

Arial, thanks for your thorough analysis.  The ToolBarManager in framework/ is applying some magic to command handling and dispatching.  Something that I previously thought would be done for every command, not just the ones in the toolbars.  The SvtCommandOptions() singleton is used to determine whether a command is disabled.  If it is not disabled a command is dispatched via XDispatch::dispatch() and not via SfxDispatcher::Execute().

I still have to find out whether the use of SfxDispatcher::Execute(), which is used throughout the office for the majority of slot calls, generally bypasses the framework command processing or if that happens only in the sidebar panels.

Anyway, fixing this should not be hard only tedious.  Write some helper/framework code and use it in all panels.

s/Arial/Ariel/

I made some experiments with exporting framework::ToolBarManager and use it to create and handle tool bars in all property panels.  This could replace a lot of the code that manually processes slot calls (eg the explicit handling of the 'Bold' command). But this would require the use of using tool box controls for all tool bar items that have drop downs or non-standard content.  These tool box controls can be registered via Controller.xcu.

This is a lot of work, but should make the property panel code much cleaner.

(In reply to comment #10)
> Arial, thanks for your thorough analysis.  The ToolBarManager in framework/
> is applying some magic to command handling and dispatching.  

In the generic case (no magic), every toolbar item in the toolbar is controlled by a UNO based toolbar controller (even those in svx are UNO based, derived from svt::ToolboxController; see http://wiki.openoffice.org/wiki/User:Arielch/UI_Controllers#ToolBar_Controllers - rather old but still valid in the general overview), see framework::ToolBarManager::CreateControllers() where the toolbar is iterated, and for every item a toolbar controller is created.

This means:

a) getting feature state updates from the dispatch object queried at the XFrame as dispatch provider

b) dispatching the UNO commnad with the dispatch object queried at the XFrame

> Something that
> I previously thought would be done for every command, not just the ones in
> the toolbars.  The SvtCommandOptions() singleton is used to determine
> whether a command is disabled.  If it is not disabled a command is
> dispatched via XDispatch::dispatch() and not via SfxDispatcher::Execute().

SvtCommandOptions has two main uses in the framework module:

a) when the dispatch providers (the Desktop and the Frame) get queried for a dispatch object, they check before if the command is disabled; if so, they return an empty reference; this has the effect that commands disabled by configuration are disabled in the UI: every item in every UI element has a controller that is a status listener at the dispatch object it queries from the XFrame, when it queries the dispatch provider for a dispatch object for its command, and the dispatch object is empty, the UI element controller disables its item.

b) the UI element managers use SvtCommandOptions to look up the command of every item, and when the command is disabled, the UI elemenet manager hides it from UI element (and to save resources, does not create a UI element controller for this item).

> I still have to find out whether the use of SfxDispatcher::Execute(), which
> is used throughout the office for the majority of slot calls, generally
> bypasses the framework command processing or if that happens only in the
> sidebar panels.

In this case I like the layers metaphor. The SfxDispatcher is used all over the office but at the lower level, for example when a shell executes an slot. The shell interface definition defines which function in the shell should be called to retrieve the slot status, and which to execute the slot.

But on top of this, you have the UNO layer, whose "central" object the css.frame.XFrame. In the UNO layer, when you want to know the state of a feature, represented by a UNO command, you have to query for a dispatch object at the XFrame. If the XFrame returns a valid dispatch object, you add an status listener at this dispatch object to get feature state updates (the approach is the same in every UI element controller, though the behavior may vary: the menu items are only visible when the menu is activated, so there is no sense in keep listening for feature status updates, thus the menu item controller adds itself as status listener at the dispatch object, and removes itself immediately, just to get a single feature update. Toolbar and statusbar items controllers keep listening for feature updates at the dispatch object, because they are visible longer than menu items, and thus need to constantly reflect the state of the command they control).

When the UNO layer provides a valid dispatch object, all will end up calling the functions defined in the respective shell interface, to retrieve the slot state, and to execute it. But here is where the Symphony implementation fails: everything is implemented in the old sfx2 framework, at the lower level, state retrieval and command execution is not mediated by the UNO frame; thus, when the UNO layer decides to return no dispatch object at all (because the command is disabled), or to return a different one (because it was intercepted), the sidebar is left out this game.

The only way not to bypass the UNO layer is controlling *every item* in the sidebar by a UNO based UI element controller that controls a UI element identified by a UNO command (this point of the UNO command being very important to solve other bugs in the Symphony implementation, see below), not a class derived from the SfxControllerItem.

In the end, the sidebar is a UI element too, it must follow the general UNO approach by the other UI element managers and controllers, not only in this particular issue, and the other related, the dispatch provider interception, but in a more general sense - I mean, there are several other bugs in the Symphony implementation that will go away by using the approach taken in the framework:

- there is a duplication of strings to be translated: just take as example all the strings for the toolbox items in RID_SIDEBAR_TEXT_PANEL. In the framework, a UI element item has a command, you retrieve the item label from the command. Following this approach, you simply use a single string in XxxCommands.xcu

- the sidebar items have HelpIDs of their own. This is a duplication too, someone will have to sit down and write help content so that pressing F1 on a sidebar item opens the respective help, and extended tooltips display it too. This can be simply solved by using UNO commands instead of brand new HelpIDs: for example, not HID_PPROPERTYPANEL_TEXT_TBI_BOLD but ".uno:Bold". Here again, the UNO command is the main entry point, the Online Help is able to retrieve the help text for a UNO command, no need to create new help content for the sidebar items that are already available on other UI elements

- hard-coded shortcuts, as in STR_QH_BOLD, STR_QH_ITALIC, STR_QH_UNDERLINE, is error prone, will not reflect user customization. The toolbar manager uses other approach (see ToolBarManager::RetrieveShortcut used when the toolbar is constructed from the XML structure, in ToolBarManager::FillToolbar() ).

In short, using UNO commands, at least where it seems more easy, in the sidebar's ToolBoxes, will solve several issues.

Thanks for the explanation.  Some details where new to me.

I agree with you that using the UNO way to handle and dispatch
commands is the best way to go.  But while the UNO command concept is
not too bad, its implementation is lacking in some areas.  The biggest
problem is that today most commands are dispatched as slot calls.  I
always thought that the framework would internally convert them into
commands and then handle them like any other command dispatch.  This
is done to some degree (a slot <id> is converted into a "slot:<id>"
command) but apparently a detail like the disabling of commands is not
applied.

I made an experiment with dispatching a disabled command and still got
an XDispatch object.  Your explanation confirms my belief that I
should have gotten an empty Reference.  But my experiment was not very
thorough, I should repeat it.

Then there is the matter of finding the right handler for a
slot/command.  Sfx with its shells allows more fine control than UNO
with its chain of dispatchers.

And there are toolbars and toolboxes.  Toolbars apply what I
called magic before: check for each item if it is disabled.
A toolbox, the base class of the toolbar, does not do this.  Disabling
commands looks like feature that was well meant but poorly
implemented.


The solution to the problem of disabled commands is dictated by
pragmatism.  The Symphony implementation obviously is not very
good.  But there is not enough time to refactor every panel.
Because I forgot to change the owner of this issue to me I saw
your last comment a bit late and have created a "solution"
similar to the svt::ToolboxController.  I have to check if I can
switch to that without spending too much time on it.  And due to
time constraints I can at the moment only convert the text
property panel.  I am not happy with this, but bringing all panels into a state that we are all happy with would take weeks or months.

"af" committed SVN revision 1469765 into trunk:
121960: Extend the sidebar::ControllerItem to check Disabled state.  Converte...

I have checked in my changes without using svt::ToolboxController.  I have extended sfx2::sidebar::ControllerItem to check whether a command is disabled when the ControllerItem constructor is provided with the command name and an XFrame object.  ControllerItem makes use of new class CommandInfoProvider to determine the label plus keyboard accelerator.  Now the lables of buttons for subscript and superscript of the TextPropertyPanel show proper shortcuts.

Reasons for not using svt::ToolboxController:
- ToolboxController is UNO only.  Using it would require a complete refactoring for each panel.
- Its header is poorly documented.
- It is almost 900 lines long and does not provide much beside state updates. Especially no labels.


Btw, I was wrong earlier.  queryDispatch really does return an empty reference for a command that is disabled.  But that also means that when a command becomes enabled then this state change is not broadcast.  This is the reason why the text toolbar will show eg the bold button only after a restart while the modified TextPropertyPanel updates the state immediately.

"af" committed SVN revision 1470943 into trunk:
121960: Fixed typo that made the superscript button look like the subscript b...

After some work in this area the best way to address this issue is probably the (re)use of existing tool bar controllers where a panel provides functionality that is already present in any tool bar.  This approach works well for the InsertShapePanel and the color controllers in the TextPropertyPanel.  Drawbacks are:

- A complete conversion will take a lot of time.

- For some panel controls we will need new toolbar controllers because
   a) the functionality does not yet exist in any toolbar or
   b) the drop downs in the panel look differently from those of the toolbars.

I don't think that we can implement this solution for the 4.0 release.

I further improved the creation of toolbox/toolbar controllers.  The ultimate goal, however, should be to use the toolbarmanger class from framework/ directly, not just borrowing code from it.  But I don't want to change this class before the 4.0 release.

The original motivation for the changes was to use standard toolbar controllers for the font selection and font size controls in the TextPropertyPanel.  This in itself is not hard to do but the buttons for increasing and decreasing the font size are making problems.  The code for all these controls is very interwoven.  In Writer and Draw/Impress the standard controls for increase/decrease could be used, their disabling when the font size reaches an upper or lower limit (72/4 pt in Writer, 0.2/999.9 pt in Draw/Impress) had to be added.  For Calc, however, this does not work because there are no slots/commands for increase/decrease.

The seemingly simple change to use standard controllers would require a lot of code changes and new features.  I do not have the time to do this now and am not willing to risk and regressions caused by such changes.

We will have to fix and problems with disabled or intercepted (bug 122024) on demand when there is a concrete problem.  For the time being I therefore remove this issue as blocker of bug 121420.

"af" committed SVN revision 1483307 into trunk:
121960: Improve creation of toolbox/toolbar controllers.

Reset assigne to the default "issues@openoffice.apache.org".