[Libreoffice-commits] core.git: toolkit/source

[Justin Luth (via logerrit)]

 toolkit/source/controls/stdtabcontroller.cxx |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

New commits:
commit 5cf057c3365a0feafc8f2e4f4a9e24d69a581999
Author:     Justin Luth <justin_luth at sil.org>
AuthorDate: Sat Jul 6 13:42:12 2019 +0300
Commit:     Justin Luth <justin_luth at sil.org>
CommitDate: Fri Jul 19 06:14:12 2019 +0200

    tdf#125609 toolkit: don't use XTabController::getControls
    
    Calling XTabController::getControls was supposed to give
    performance improvements (coded in OOo), but it
    pulls cached information which is not up to date if
    this listener for elementInserted events is handled before
    the formController's listener. It is missing the most recently
    created control - and thus it never sees the last control
    in the form, and fails to create the radio group.
    
    Additionally, when all of the controls are not yet
    created, this function seems to be designed to catch that
    and immediately return. With the "optimization" the
    missing controls were never noticed, and so unnecessary
    processing continued - a performance detriment while
    the form is being built.
    
    My impresssion is that the local getControl() function
    is not terribly inefficient, so the performance impact
    seems minimal, especially since it now only
    makes the call once and caches the result itself.
    
    Since not-yet-peered controls cause the function to again
    terminate early (as it was designed to do),
    this may have unintended side effects,
    in case anything was designed in the past 10+ years
    expecting the old behaviour, so I have no intention
    of back-porting this.
    
    Change-Id: Ica8ddab69043a30b23d008cd8db5df1c13b94ad2
    Reviewed-on: https://gerrit.libreoffice.org/75163
    Tested-by: Jenkins
    Reviewed-by: Justin Luth <justin_luth at sil.org>

diff --git a/toolkit/source/controls/stdtabcontroller.cxx b/toolkit/source/controls/stdtabcontroller.cxx
index 7f89093a8dde..61900b4d76ce 100644
--- a/toolkit/source/controls/stdtabcontroller.cxx
+++ b/toolkit/source/controls/stdtabcontroller.cxx
@@ -308,18 +308,17 @@ void StdTabController::activateTabOrder(  )
     if ( !xC.is() || !xVclContainerPeer.is() )
         return;
 
-    // This may return a TabController, which returns desired list of controls faster
-    Reference< XTabController >  xTabController(static_cast< ::cppu::OWeakObject* >(this), UNO_QUERY);
-
     // Get a flattened list of controls sequences
     Sequence< Reference< XControlModel > > aModels = mxModel->getControlModels();
     Sequence< Reference< XWindow > > aCompSeq;
     Sequence< Any> aTabSeq;
 
-    // DG: For the sake of optimization, retrieve Controls from getControls(),
-    // this may sound counterproductive, but leads to performance improvements
-    // in practical scenarios (Forms)
-    Sequence< Reference< XControl > > aControls = xTabController->getControls();
+    // Previously used aControls = xTabController->getControls() "for the sake of optimization",
+    // but that list isn't valid during the creation phase (missing last created control) because
+    // listenermultiplexer.cxx handles fmvwimp::elementinserted before formcontroller::elementInserted
+    // Perhaps other places using the same optimization need to be reviewed?  (tdf#125609)
+    Sequence< Reference< XControl > > aCachedControls = getControls();
+    Sequence< Reference< XControl > > aControls = aCachedControls;
 
     // #58317# Some Models may be missing from the Container. Plus there is a
     // autoTabOrder call later on.
@@ -337,7 +336,7 @@ void StdTabController::activateTabOrder(  )
     {
         mxModel->getGroup( nG, aThisGroupModels, aName );
 
-        aControls = xTabController->getControls();
+        aControls = aCachedControls;
             // ImplCreateComponentSequence has a really strange semantics regarding it's first parameter:
             // upon method entry, it expects a super set of the controls which it returns
             // this means we need to completely fill this sequence with all available controls before