tdf#134887 Rework last item handling in NewToolbarController

Just store the last used menu id, and consistently get all the
information (command name, image id, target frame etc.) from the menu
instance. Also implement the XSubToolbarController interface, so we get
a chance to update the image on icon size or theme change.

The initial action and icon of the button are based on the url passed
by the .uno:AddDirect status update, given that there is a corresponding
menu item. The only defined fallback now is to .uno:AddDirect itself for
both executing and getting the icon, but this shouldn't happen normally,
as I fixed all the affected cases I found so far, namely Base (including
windows like table design etc.) and the Basic IDE.

Actually these cases were partially broken anyway, and as a result now
completely fixed:

- In Base main app window, the button used to default to a new Writer
document, although its tooltip advertises Ctrl+N as the kb shortcut, and
pressing that shortcut actually opens the database creation wizard (as
expected).

- In the Basic IDE, the button was working, but not the Ctrl+N shortcut.

Change-Id: I6537b3f38e7d7001ec1b39d78285445464fd8cbe
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/99514
Tested-by: Jenkins
Reviewed-by: Maxim Monastirsky <momonasmon@gmail.com>

8 files changed, 103 insertions, 125 deletions

diff --git a/basctl/sdi/baside.sdi b/basctl/sdi/baside.sdi
index 447e5ad6fd71..455924549556 100644
--- a/basctl/sdi/baside.sdi
+++ b/basctl/sdi/baside.sdi
@@ -206,6 +206,12 @@ shell basctl_Shell
         StateMethod = GetState;
     ]
 
+    SID_NEWDOCDIRECT
+    [
+        ExecMethod  = ExecuteGlobal;
+        StateMethod = GetState;
+    ]
+
     SID_SAVEDOC
     [
         ExecMethod  = ExecuteGlobal;
diff --git a/basctl/source/basicide/basides1.cxx b/basctl/source/basicide/basides1.cxx
index 12399e823019..e42ad914390a 100644
--- a/basctl/source/basicide/basides1.cxx
+++ b/basctl/source/basicide/basides1.cxx
@@ -40,6 +40,7 @@
 #include <svl/srchdefs.hxx>
 #include <sal/log.hxx>
 #include <osl/diagnose.h>
+#include <sfx2/app.hxx>
 #include <sfx2/bindings.hxx>
 #include <sfx2/childwin.hxx>
 #include <sfx2/dinfdlg.hxx>
@@ -270,6 +271,14 @@ void Shell::ExecuteGlobal( SfxRequest& rReq )
     sal_uInt16 nSlot = rReq.GetSlot();
     switch ( nSlot )
     {
+        case SID_NEWDOCDIRECT:
+        {
+            // we do not have a new document factory,
+            // so just forward to a fallback method.
+            SfxGetpApp()->ExecuteSlot(rReq);
+        }
+        break;
+
         case SID_BASICSTOP:
         {
             // maybe do not simply stop if on breakpoint!
@@ -770,6 +779,13 @@ void Shell::GetState(SfxItemSet &rSet)
     {
         switch ( nWh )
         {
+            case SID_NEWDOCDIRECT:
+            {
+                // we do not have a new document factory,
+                // so just forward to a fallback method.
+                SfxGetpApp()->GetSlotState(nWh, nullptr, &rSet);
+            }
+            break;
             case SID_DOCINFO:
             {
                 rSet.DisableItem( nWh );
diff --git a/dbaccess/source/ui/app/AppController.cxx b/dbaccess/source/ui/app/AppController.cxx
index b7036d149e25..958d5d9c7b4f 100644
--- a/dbaccess/source/ui/app/AppController.cxx
+++ b/dbaccess/source/ui/app/AppController.cxx
@@ -532,6 +532,10 @@ FeatureState OApplicationController::GetState(sal_uInt16 _nId) const
     {
         switch (_nId)
         {
+            case SID_NEWDOCDIRECT:
+                aReturn.bEnabled = true;
+                aReturn.sTitle = "private:factory/sdatabase";
+                break;
             case SID_OPENURL:
                 aReturn.bEnabled = true;
                 if ( m_xModel.is() )
@@ -1031,17 +1035,25 @@ void OApplicationController::Execute(sal_uInt16 _nId, const Sequence< PropertyVa
                     }
                 }
                 break;
+            case SID_NEWDOCDIRECT:
             case SID_OPENDOC:
                 {
                     Reference < XDispatchProvider > xProv( getFrame(), UNO_QUERY );
                     if ( xProv.is() )
                     {
                         URL aURL;
-                        aURL.Complete = ".uno:Open";
+                        OUString aTarget;
+                        if ( _nId == SID_NEWDOCDIRECT )
+                        {
+                            aURL.Complete = "private:factory/sdatabase?Interactive";
+                            aTarget = "_default";
+                        }
+                        else
+                            aURL.Complete = ".uno:Open";
 
                         if ( m_xUrlTransformer.is() )
                             m_xUrlTransformer->parseStrict( aURL );
-                        Reference < XDispatch > xDisp = xProv->queryDispatch( aURL, OUString(), 0 );
+                        Reference < XDispatch > xDisp = xProv->queryDispatch( aURL, aTarget, 0 );
                         if ( xDisp.is() )
                             xDisp->dispatch( aURL, Sequence < PropertyValue >() );
                     }
@@ -1340,6 +1352,7 @@ void OApplicationController::describeSupportedFeatures()
 {
     OGenericUnoController::describeSupportedFeatures();
 
+    implDescribeSupportedFeature( ".uno:AddDirect",          SID_NEWDOCDIRECT,          CommandGroup::APPLICATION );
     implDescribeSupportedFeature( ".uno:Save",               ID_BROWSER_SAVEDOC,        CommandGroup::DOCUMENT );
     implDescribeSupportedFeature( ".uno:SaveAs",             ID_BROWSER_SAVEASDOC,      CommandGroup::DOCUMENT );
     implDescribeSupportedFeature( ".uno:SendMail",           SID_MAIL_SENDDOC,          CommandGroup::DOCUMENT );
diff --git a/framework/source/uielement/popuptoolbarcontroller.cxx b/framework/source/uielement/popuptoolbarcontroller.cxx
index ea76be37887c..2f35ee41cbfb 100644
--- a/framework/source/uielement/popuptoolbarcontroller.cxx
+++ b/framework/source/uielement/popuptoolbarcontroller.cxx
@@ -396,7 +396,7 @@ public:
     virtual void SAL_CALL initialize( const css::uno::Sequence< css::uno::Any >& aArguments ) override;
 
     // XSubToolbarController
-    // Ugly HACK to cause ToolBarManager ask our controller for updated image, in case of icon theme change.
+    // Make ToolBarManager ask our controller for updated image, in case of icon theme change.
     virtual sal_Bool SAL_CALL opensSubToolbar() override;
     virtual OUString SAL_CALL getSubToolbarName() override;
     virtual void SAL_CALL functionSelected( const OUString& aCommand ) override;
@@ -580,7 +580,7 @@ css::uno::Sequence< OUString > SaveToolbarController::getSupportedServiceNames()
     return {"com.sun.star.frame.ToolbarController"};
 }
 
-class NewToolbarController : public PopupMenuToolbarController
+class NewToolbarController : public cppu::ImplInheritanceHelper<PopupMenuToolbarController, css::frame::XSubToolbarController>
 {
 public:
     explicit NewToolbarController( const css::uno::Reference< css::uno::XComponentContext >& rxContext );
@@ -594,18 +594,26 @@ public:
 
     void SAL_CALL initialize( const css::uno::Sequence< css::uno::Any >& aArguments ) override;
 
+    // XSubToolbarController
+    // Make ToolBarManager ask our controller for updated image, in case of icon theme change.
+    sal_Bool SAL_CALL opensSubToolbar() override { return true; }
+    OUString SAL_CALL getSubToolbarName() override { return OUString(); }
+    void SAL_CALL functionSelected( const OUString& ) override {}
+    void SAL_CALL updateImage() override;
+
 private:
     void functionExecuted( const OUString &rCommand ) override;
     void SAL_CALL statusChanged( const css::frame::FeatureStateEvent& rEvent ) override;
     void SAL_CALL execute( sal_Int16 KeyModifier ) override;
-    void setItemImage( const OUString &rCommand );
+    sal_uInt16 getMenuIdForCommand( const OUString &rCommand );
 
-    OUString m_aLastURL;
+    sal_uInt16 m_nMenuId;
 };
 
 NewToolbarController::NewToolbarController(
     const css::uno::Reference< css::uno::XComponentContext >& xContext )
-    : PopupMenuToolbarController( xContext )
+    : ImplInheritanceHelper( xContext )
+    , m_nMenuId( 0 )
 {
 }
 
@@ -641,8 +649,8 @@ void SAL_CALL NewToolbarController::statusChanged( const css::frame::FeatureStat
         try
         {
             // set the image even if the state is not a string
-            // this will set the image of the default module
-            setItemImage( aState );
+            // the toolbar item command will be used as a fallback
+            functionExecuted( aState );
         }
         catch (const css::ucb::CommandFailedException&)
         {
@@ -658,154 +666,90 @@ void SAL_CALL NewToolbarController::statusChanged( const css::frame::FeatureStat
 void SAL_CALL NewToolbarController::execute( sal_Int16 /*KeyModifier*/ )
 {
     osl::MutexGuard aGuard( m_aMutex );
-    if ( !m_aLastURL.getLength() )
-        return;
 
-    OUString aTarget( "_default" );
-    if ( m_xPopupMenu.is() )
+    OUString aURL, aTarget;
+    if ( m_xPopupMenu.is() && m_nMenuId )
     {
         // TODO investigate how to wrap Get/SetUserValue in css::awt::XMenu
-        MenuAttributes* pMenuAttributes( nullptr );
-        VCLXPopupMenu*  pTkPopupMenu =
-            static_cast<VCLXPopupMenu *>( comphelper::getUnoTunnelImplementation<VCLXMenu>( m_xPopupMenu ) );
-
         SolarMutexGuard aSolarMutexGuard;
-        PopupMenu* pVCLPopupMenu = pTkPopupMenu ?
-            dynamic_cast< PopupMenu * >( pTkPopupMenu->GetMenu() ) : nullptr;
-
-        if ( pVCLPopupMenu )
-            pMenuAttributes = static_cast< MenuAttributes* >(
-                pVCLPopupMenu->GetUserValue( pVCLPopupMenu->GetCurItemId() ) );
+        Menu* pVclMenu = comphelper::getUnoTunnelImplementation<VCLXMenu>( m_xPopupMenu )->GetMenu();
+        aURL = pVclMenu->GetItemCommand( m_nMenuId );
 
+        MenuAttributes* pMenuAttributes( static_cast<MenuAttributes*>( pVclMenu->GetUserValue( m_nMenuId ) ) );
         if ( pMenuAttributes )
             aTarget = pMenuAttributes->aTargetFrame;
+        else
+            aTarget = "_default";
     }
+    else
+        aURL = m_aCommandURL;
 
     css::uno::Sequence< css::beans::PropertyValue > aArgs( 1 );
     aArgs[0].Name = "Referer";
     aArgs[0].Value <<= OUString( "private:user" );
 
-    dispatchCommand( m_aLastURL, aArgs, aTarget );
+    dispatchCommand( aURL, aArgs, aTarget );
 }
 
 void NewToolbarController::functionExecuted( const OUString &rCommand )
 {
-    setItemImage( rCommand );
+    m_nMenuId = getMenuIdForCommand( rCommand );
+    updateImage();
 }
 
-/**
-    it return the existing state of the given URL in the popupmenu of this toolbox control.
-
-    If the given URL can be located as an action command of one menu item of the
-    popup menu of this control, we return sal_True. Otherwise we return sal_False.
-    Further we return a fallback URL, in case we have to return sal_False. Because
-    the outside code must select a valid item of the popup menu every time ...
-    and we define it here. By the way this method was written to handle
-    error situations gracefully. E.g. it can be called during creation time
-    but then we have no valid menu. For this case we know another fallback URL.
-    Then we return the private:factory/ URL of the default factory.
-
-    @param  rPopupMenu
-                points to the popup menu, on which item we try to locate the given URL
-                Can be NULL! Search will be suppressed then.
-
-    @param  sURL
-                the URL for searching
-
-    @param  sFallback
-                contains the fallback URL in case we return FALSE
-                Must point to valid memory!
-
-    @param  aImage
-                contains the image of the menu for the URL.
-
-    @return sal_True - if URL could be located as an item of the popup menu.
-            sal_False - otherwise.
-*/
-bool Impl_ExistURLInMenu(
-    const css::uno::Reference< css::awt::XPopupMenu > &rPopupMenu,
-    OUString &sURL,
-    OUString &sFallback,
-    Image &aImage )
+sal_uInt16 NewToolbarController::getMenuIdForCommand( const OUString &rCommand )
 {
-    bool bValidFallback( false );
-    sal_uInt16 nCount( 0 );
-    if ( rPopupMenu.is() )
+    if ( m_xPopupMenu.is() && !rCommand.isEmpty() )
     {
-        nCount = rPopupMenu->getItemCount();
-        if (nCount != 0 && sURL.getLength() )
+        Menu* pVclMenu( comphelper::getUnoTunnelImplementation<VCLXMenu>( m_xPopupMenu )->GetMenu() );
+        sal_uInt16 nCount = pVclMenu->GetItemCount();
+        for ( sal_uInt16 n = 0; n < nCount; ++n )
         {
-            for ( sal_uInt16 n = 0; n < nCount; ++n )
-            {
-                sal_uInt16 nId = rPopupMenu->getItemId( n );
-                OUString aCmd( rPopupMenu->getCommand( nId ) );
-
-                if ( !bValidFallback && aCmd.getLength() )
-                {
-                    sFallback = aCmd;
-                    bValidFallback = true;
-                }
-
-                // match even if the menu command is more detailed
-                // (maybe an additional query) #i28667#
-                if ( aCmd.match( sURL ) )
-                {
-                    sURL = aCmd;
-                    const css::uno::Reference< css::graphic::XGraphic > xGraphic(
-                        rPopupMenu->getItemImage( nId ) );
-                    if ( xGraphic.is() )
-                        aImage = Image( xGraphic );
-                    return true;
-                }
-            }
-        }
-    }
+            sal_uInt16 nId = pVclMenu->GetItemId( n );
+            OUString aCmd( pVclMenu->GetItemCommand( nId ) );
 
-    if ( !bValidFallback )
-    {
-        OUStringBuffer aBuffer;
-        aBuffer.append( "private:factory/" );
-        aBuffer.append( SvtModuleOptions().GetDefaultModuleName() );
-        sFallback = aBuffer.makeStringAndClear();
+            // match even if the menu command is more detailed
+            // (maybe an additional query) #i28667#
+            if ( aCmd.match( rCommand ) )
+                return nId;
+        }
     }
 
-    return false;
+    return 0;
 }
 
-/** We accept URL's here only, which exist as items of our internal popup menu.
-    All other ones will be ignored and a fallback is used.
- */
-void NewToolbarController::setItemImage( const OUString &rCommand )
+void SAL_CALL NewToolbarController::updateImage()
 {
     SolarMutexGuard aSolarLock;
     VclPtr< ToolBox> pToolBox = static_cast< ToolBox* >( VCLUnoHelper::GetWindow( getParent() ).get() );
     if ( !pToolBox )
         return;
 
-    OUString aURL = rCommand;
-    OUString sFallback;
-    Image aMenuImage;
+    OUString aURL, aImageId;
+    if ( m_xPopupMenu.is() && m_nMenuId )
+    {
+        Menu* pVclMenu = comphelper::getUnoTunnelImplementation<VCLXMenu>( m_xPopupMenu )->GetMenu();
+        aURL = pVclMenu->GetItemCommand( m_nMenuId );
 
-    bool bValid( Impl_ExistURLInMenu( m_xPopupMenu, aURL, sFallback, aMenuImage ) );
-    if ( !bValid )
-        aURL = sFallback;
+        MenuAttributes* pMenuAttributes( static_cast<MenuAttributes*>( pVclMenu->GetUserValue( m_nMenuId ) ) );
+        if ( pMenuAttributes )
+            aImageId = pMenuAttributes->aImageId;
+    }
+    else
+        aURL = m_aCommandURL;
 
-    bool bBig = SvtMiscOptions().AreCurrentSymbolsLarge();
+    INetURLObject aURLObj( aImageId.isEmpty() ? aURL : aImageId );
+    vcl::ImageType eImageType( pToolBox->GetImageSize() );
+    bool bBig( eImageType != vcl::ImageType::Small );
+    Size aPreferredSize( bBig ? pToolBox->GetDefaultImageSize() : Size() );
+    Image aImage = SvFileInformationManager::GetImageNoDefault( aURLObj, bBig, aPreferredSize );
+    if ( !aImage )
+        aImage = vcl::CommandInfoProvider::GetImageForCommand( aURL, m_xFrame, eImageType );
 
-    INetURLObject aURLObj( aURL );
-    Size aPreferredSize(bBig ? pToolBox->GetDefaultImageSize() : Size());
-    Image aImage = SvFileInformationManager::GetImageNoDefault(aURLObj, bBig, aPreferredSize);
-    if (!aImage)
-    {
-        aImage = !!aMenuImage ? aMenuImage : SvFileInformationManager::GetImage(aURLObj, bBig, aPreferredSize);
-    }
-    // if everything failed, just use the image associated with the toolbar item command
     if ( !aImage )
         return;
 
     pToolBox->SetItemImage( m_nToolBoxId, aImage );
-
-    m_aLastURL = aURL;
 }
 
 }
diff --git a/include/sfx2/app.hxx b/include/sfx2/app.hxx
index f08855ee8d18..481dc9dd5650 100644
--- a/include/sfx2/app.hxx
+++ b/include/sfx2/app.hxx
@@ -188,6 +188,7 @@ public:
     // Slot Methods
     // TODO/CLEANUP: still needed?
     SAL_DLLPRIVATE void         NewDocDirectExec_Impl(SfxRequest &);
+    SAL_DLLPRIVATE static void  NewDocDirectState_Impl(SfxItemSet &);
     SAL_DLLPRIVATE void         NewDocExec_Impl(SfxRequest &);
     SAL_DLLPRIVATE void         OpenDocExec_Impl(SfxRequest &);
     SAL_DLLPRIVATE void         OpenRemoteExec_Impl(SfxRequest &);
diff --git a/officecfg/registry/data/org/openoffice/Office/Accelerators.xcu b/officecfg/registry/data/org/openoffice/Office/Accelerators.xcu
index 06bfd060d9da..d54fabafaebc 100644
--- a/officecfg/registry/data/org/openoffice/Office/Accelerators.xcu
+++ b/officecfg/registry/data/org/openoffice/Office/Accelerators.xcu
@@ -500,14 +500,7 @@ Ctrl+Shift+e aka E_SHIFT_MOD1 under GTK/IBUS is for some emoji thing
           </prop>
         </node>
       </node>
-      <node oor:name="com.sun.star.sdb.OfficeDatabaseDocument" oor:op="replace">
-        <node oor:name="N_MOD1" oor:op="replace">
-          <prop oor:name="Command">
-            <value xml:lang="x-no-translate">I10N SHORTCUTS - NO TRANSLATE</value>
-            <value xml:lang="en-US">private:factory/sdatabase?Interactive</value>
-          </prop>
-        </node>
-      </node>
+      <node oor:name="com.sun.star.sdb.OfficeDatabaseDocument" oor:op="replace"/>
       <node oor:name="com.sun.star.sdb.DataSourceBrowser" oor:op="replace">
         <node oor:name="F_MOD1" oor:op="replace">
           <prop oor:name="Command">
diff --git a/sfx2/sdi/appslots.sdi b/sfx2/sdi/appslots.sdi
index c12eb098b37e..be81e3e97dd4 100644
--- a/sfx2/sdi/appslots.sdi
+++ b/sfx2/sdi/appslots.sdi
@@ -222,6 +222,7 @@ shell SfxApplication
     SID_NEWDOCDIRECT // ole(no) api(no)
     [
         ExecMethod = NewDocDirectExec_Impl ;
+        StateMethod = NewDocDirectState_Impl ;
     ]
     SID_CLOSEDOCS
     [
diff --git a/sfx2/source/appl/appopen.cxx b/sfx2/source/appl/appopen.cxx
index 073f25121a1b..f888ebf1981b 100644
--- a/sfx2/source/appl/appopen.cxx
+++ b/sfx2/source/appl/appopen.cxx
@@ -431,6 +431,10 @@ void SfxApplication::NewDocDirectExec_Impl( SfxRequest& rReq )
         rReq.SetReturnValue( SfxFrameItem( 0, pItem->GetFrame() ) );
 }
 
+void SfxApplication::NewDocDirectState_Impl( SfxItemSet &rSet )
+{
+    rSet.Put(SfxStringItem(SID_NEWDOCDIRECT, "private:factory/" + SvtModuleOptions().GetDefaultModuleName()));
+}
 
 void SfxApplication::NewDocExec_Impl( SfxRequest& rReq )
 {