Issue 116455 - toolbar Style settings are not made persistent.
toolbar Style settings are not made persistent.
Status: RESOLVED FIXED
Product: General
Classification: Code
Component: ui
DEV300m97
PC Windows XP
: P3 trivial (vote)
: 4.0.0
Assigned To: Ariel Constenla-Haile
issues@framework
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-13 19:40 UTC by Regina Henschel
Modified: 2013-01-05 18:44 UTC (History)
2 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
document to test toolbar setting (15.27 KB, application/vnd.oasis.opendocument.text)
2011-01-13 19:42 UTC, Regina Henschel
no flags Details
Oops, toolbar was not embedded. Now the correct file with embedded toolbar (15.65 KB, application/vnd.oasis.opendocument.text)
2011-01-14 23:24 UTC, Regina Henschel
no flags Details
patch fixing issue (1.22 KB, patch)
2011-01-17 05:36 UTC, Ariel Constenla-Haile
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Regina Henschel 2011-01-13 19:40:30 UTC
Open attached document.
Open “Customize” dialog of the toolbar “MyMacro”. (It should flow.)
Click on “Toolbar” in the upper dialog part.
Choose “Icons & Text”. OK
Save document.
Reload.
Notice, the toolbar setting has changed to “Icons only”.
Comment 1 Regina Henschel 2011-01-13 19:42:18 UTC
Created attachment 75557 [details]
document to test toolbar setting
Comment 2 Ariel Constenla-Haile 2011-01-14 22:48:56 UTC
@regina: the attached document has no custom toolbar inside it, I guess you
meant to create a custom toolbar, named "MyMacro" with a toolbar button
dispatching the macro inside the document
Comment 3 Regina Henschel 2011-01-14 23:24:11 UTC
Created attachment 75566 [details]
Oops, toolbar was not embedded. Now the correct file with embedded toolbar
Comment 4 Ariel Constenla-Haile 2011-01-17 05:36:02 UTC
Created attachment 75577 [details]
patch fixing issue
Comment 5 Ariel Constenla-Haile 2011-01-17 05:57:45 UTC
@cd: please take a look at the attached patch. Short explanation follows:

set a break point on ToolbarSaveInData::SetSystemStyle
http://hg.services.openoffice.org/DEV300/file/DEV300_m97/cui/source/customize/cfg.cxx#l3829

The for loops over all properties and never finds the ITEM_DESCRIPTOR_STYLE
property.
The problem is that the toolbar Style settings are stored in the WindowState
data, but when the LayoutManager (now the ToolbarLayoutManager) first writes
this WindowState data it misses the ITEM_DESCRIPTOR_STYLE property.
Comment 6 Ariel Constenla-Haile 2011-01-17 06:01:50 UTC
@cd: the code to be fixed is ToolbarLayoutManager::implts_writeWindowStateData
http://hg.services.openoffice.org/DEV300/file/DEV300_m97/framework/source/layoutmanager/toolbarlayoutmanager.cxx#l1671
Comment 7 carsten.driesner 2011-01-17 16:40:53 UTC
cd-<<arielch: Thanks for the patch Ariel and your investigation. I will check it
as soon as possible.
Comment 8 carsten.driesner 2011-01-21 09:39:39 UTC
cd->arielch: A first check reveals that I don't know why your patch fixes the
problem. The property "Style" is part of the schema and has a default value "0".
Therefore the code should be able to "see" the property without the need to
write it first into the user layer. I need more time to check and find a good
explanation why your fix work and why this problem occurs.
Comment 9 Ariel Constenla-Haile 2011-01-21 13:57:47 UTC
arielch->cd: ups maybe I was too cryptic about the explanation.
I debugged this down to the root of the problem, the implementation of the
com.sun.star.ui.WindowStateConfiguration service:
http://hg.services.openoffice.org/DEV300/file/DEV300_m97/framework/inc/uiconfiguration/windowstateconfiguration.hxx
http://hg.services.openoffice.org/DEV300/file/DEV300_m97/framework/source/uiconfiguration/windowstateconfiguration.cxx

As you notice, there is no problem with the configuration API. The problem is 
a) the ConfigurationAccess_WindowState caches the information
b) and how it caches that information when the toolbar is created and there is
no WindowState config. available: it only caches the properties that have been
set (this information is stored in a binary field).

See ConfigurationAccess_WindowState::insertByName

* in
http://hg.services.openoffice.org/DEV300/file/DEV300_m97/framework/source/uiconfiguration/windowstateconfiguration.cxx#l471
the WindowState is stored in the cache from the properties set in the
ToolbarlayoutManager

   471                     WindowStateInfo aWinStateInfo;
   472                     impl_fillStructFromSequence( aWinStateInfo, aPropSet );
   473                     m_aResourceURLToInfoCache.insert(
ResourceURLToInfoCache::value_type( rResourceURL, aWinStateInfo ));


* notice that impl_fillStructFromSequence stores bitwise which properties are set

* then the WindowState is inserted in the OOo regsitry

   475                     // insert must be write-through => insert element
into configuration
   476                     Reference< XNameContainer > xNameContainer(
m_xConfigAccess, UNO_QUERY );
   477                     if ( xNameContainer.is() )
   478                     {
   479                         Reference< XSingleServiceFactory > xFactory(
m_xConfigAccess, UNO_QUERY );
   480                         aLock.unlock();
   481 
   482                         try
   483                         {
   484                             Reference< XPropertySet > xPropSet(
xFactory->createInstance(), UNO_QUERY );
   485                             if ( xPropSet.is() )
   486                             {
   487                                 Any a;
   488                                 impl_putPropertiesFromStruct(
aWinStateInfo, xPropSet );
   489                                 a <<= xPropSet;
   490                                 xNameContainer->insertByName(
rResourceURL, a );
   491                                 Reference< XChangesBatch > xFlush(
xFactory, UNO_QUERY );
   492                                 if ( xFlush.is() )
   493                                     xFlush->commitChanges();
   494                             }
   495                         }
   496                         catch ( Exception& )
   497                         {
   498                         }
   499                     }


* impl_putPropertiesFromStruct only sets the properties marked in the mask
field. Of course the configuration API takes care of default values. But the
issue here is that ConfigurationAccess_WindowState does not read them back from
the configuration, but retrieves the WindowState from the cache.
And when the toolbar is created it has no WindowState in the OOo regsitry, so
only the properties set by the ToolbarlayoutManager are cached:

see ConfigurationAccess_WindowState::getByName

   333     ResourceURLToInfoCache::const_iterator pIter =
m_aResourceURLToInfoCache.find( rResourceURL );
   334     if ( pIter != m_aResourceURLToInfoCache.end() )
   335         return impl_getSequenceFromStruct( pIter->second );
   336     else
   337     {
   338         Any a( impl_getWindowStateFromResourceURL( rResourceURL ) );
   339         if ( a == Any() )
   340             throw NoSuchElementException();
   341         else
   342             return a;


The WindowState of the newly created toolbar is on the cache with only the
properties set by the ToolbarlayoutManager. impl_getSequenceFromStruct only sets
those, but no default value at all.


I first thought of the solution of modifying
ConfigurationAccess_WindowState::insertByName and
ConfigurationAccess_WindowState::replaceByName to cache the WindowState
retrieved back from the configuration. But if we go this way, then the whole way
ConfigurationAccess_WindowState stores which properties are set in the mask
field is completely pointless, and all the code should be simplified to write
and read from the OOo regsitry, caching the whole set of properties.
Comment 10 carsten.driesner 2011-01-21 14:10:40 UTC
cd->arielch: Ok, thanks for the explanation. Although I wrote this part several
years ago I just forgot it. From that point of view your patch looks like the
best solution without rewriting much more code. Thanks for pointing it out!
Comment 11 Ariel Constenla-Haile 2013-01-05 18:43:48 UTC
Fixed in revision 1429068