Re: Question regarding Session management and toolbar state (ticket 7099)

Am 03.01.2011 um 11:28 schrieb Vincent van Ravesteijn:

>> 
>> But somehow there is a connection between visibility flags and the
>> toolbar type, isn't it? The allowauto_ flag depends on visibility.
>> This looks a little bit weird.
> 
> 
> Yes, this is just an example of bad programming practice.
> 
> In the Visibility enum, we are now mixing up flags indicating where to
> show and when to show a toolbar. This seems bad, but ok.
> 
> But then GuiToolbar::setVisibility says that all items in the
> Visibility enum >= Toolbars::MATH are auto_toggling. This is just bad,
> because someone else adds a new item to the Visibility enum and can't
> possibly know that somewhere else in the code, this new item implies a
> different behaviour because it's added to the end.
> 
> So, when the SAMEROW flag is set, we also set allow_auto_, which is a BUG.
> 
> I guess allowauto_ should be something like:
> 
> {{{
> ...
> enum Visibility {
> ....
> ALLOWAUTO = MATH | TABLE | REVIEW | MATHMACROTEMPLATE;
> ...
> }
> 
> allowauto_ = visibility & Toolbars::ALLOWAUTO;
> 
> }}}
> 
> 
>> 
>>> Furthermore, I'm not sure yet what causes the bug, because it seems
>>> that if the user sets the toolbar visibility from within LyX, this
>>> setting is lost again on the new start-up of LyX.
>> 
>> The wrong settings are saved again?
>> 
>> Perhaps it's not a failure of restoreSession but a save of wrong
>> toolbar settings. I cannot tell for sure.
> 
> You can find out if you can reproduce the bug ;).

Hmm...
I'm sure in the saved preferences all toolbar visibility settings
had a 0 assigned when I had the problem. And I can trigger the
misbehavior when saving a 0 with the property list editor.

>> In any case it shouldn't happen that visibility is restored as 0.
> 
> No, if we can't restore from QSettings, we should set the visibility
> to the default I guess.

I've prepared a patch for this based on your proposal.
The removed private name_ member in GuiToolbar.h isn't used anywhere.

 <at> Abdel: I don't have the energy to start with a git repository somewhere.
Either I apply it to svn or you or Vincent take over.

The patch is attached.

Stephan

Index: src/frontends/qt4/GuiToolbar.cpp
===================================================================
--- src/frontends/qt4/GuiToolbar.cpp	(Revision 37067)
+++ src/frontends/qt4/GuiToolbar.cpp	(Arbeitskopie)
 <at>  <at>  -90,8 +90,9  <at>  <at> 

 void GuiToolbar::setVisibility(int visibility)
 {
-	visibility_ = visibility;
-	allowauto_ = visibility_ >= Toolbars::MATH;
+	visibility_ = visibility ? visibility :
+		guiApp->toolbars().defaultVisibility(fromqstr(objectName()));
+	allowauto_ = visibility_ & Toolbars::ALLOWAUTO;
 }

 
Index: src/frontends/qt4/GuiToolbar.h
===================================================================
--- src/frontends/qt4/GuiToolbar.h	(Revision 37067)
+++ src/frontends/qt4/GuiToolbar.h	(Arbeitskopie)
 <at>  <at>  -112,8 +112,6  <at>  <at> 
 	void showEvent(QShowEvent *);

 	///
-	QString name_;
-	///
 	QList<Action *> actions_;
 	/// initial visibility flags
 	int visibility_;
Index: src/frontends/qt4/Toolbars.cpp
===================================================================
--- src/frontends/qt4/Toolbars.cpp	(Revision 37067)
+++ src/frontends/qt4/Toolbars.cpp	(Arbeitskopie)
 <at>  <at>  -370,7 +370,7  <at>  <at> 
 		}
 		toolbar_visibility_[name] = visibility;

-		if (visibility >= MATH) {
+		if (visibility & ALLOWAUTO) {
 			if (ToolbarInfo const * ti = info(name))
 				const_cast<ToolbarInfo *>(ti)->gui_name +=
 					" (" + _("auto") + ")";
Index: src/frontends/qt4/Toolbars.h
===================================================================
--- src/frontends/qt4/Toolbars.h	(Revision 37067)
+++ src/frontends/qt4/Toolbars.h	(Arbeitskopie)
 <at>  <at>  -108,7 +108,8  <at>  <at> 
 		TABLE = 256, //< show when in table
 		REVIEW = 512, //< show when change tracking is enabled
 		MATHMACROTEMPLATE = 1024, //< show in math macro template
-		SAMEROW = 2048 //place to the current row, no new line
+		SAMEROW = 2048, //place to the current row, no new line
+		ALLOWAUTO = MATH | TABLE | REVIEW | MATHMACROTEMPLATE
 	};

 	typedef std::vector<ToolbarInfo> Infos;