Le 11/08/2016 à 14:56, racoon a écrit :


Hi, Thanks and sorry for the delay. Here is an updated version. (I hope
I have not forgotten to include all files in the patch.) This time the
Lock Toolbars Positions is locked if and only if each toolbar is locked.
I guess that makes more sense.


Hi Daniel,

at last, a few comments. Some general ones first:

* Although your patch has extension .patch, it is only a diff. (The best
way to produce a patch is to use the command git format-patch which
includes the author, the timestamp and the commit log.)

* At some point the indentation is not correct (one tab too much). Have
you set up your editor properly so that it inserts the proper amount of
indentation automatically?

* I expected to find the lock toolbar option when right-clicking on the
toolbars. Do you know where to add this?

Some more specific comments below.



diff --git a/src/FuncCode.h b/src/FuncCode.h
index 04f1671..3fd0319 100644
--- a/src/FuncCode.h
+++ b/src/FuncCode.h
@@ -466,6 +466,7 @@ enum FuncCode
        LFUN_BUFFER_MOVE_PREVIOUS,      // skostysh 20150408
        LFUN_TABULAR_FEATURE,           // gm, 20151210
        LFUN_BRANCH_INVERT,             // rgheck, 20160712
+       LFUN_TOOLBAR_MOVABLE,           // daniel, 20160712
        LFUN_LASTACTION                 // end of the table
 };

diff --git a/src/LyXAction.cpp b/src/LyXAction.cpp
index 77864db..a974afa 100644
--- a/src/LyXAction.cpp
+++ b/src/LyXAction.cpp
@@ -2728,6 +2728,17 @@ void LyXAction::init()
  */
                { LFUN_TOOLBAR_TOGGLE, "toolbar-toggle", NoBuffer, Buffer },
 /*!
+* \var lyx::FuncCode lyx::LFUN_TOOLBAR_MOVABLE
+* \li Action: Toggles movability of a given toolbar between true/false.
+* \li Syntax: toolbar-movable <NAME>
+* \li Params: <NAME>: *|standard|extra|table|math|mathmacrotemplate|\n
+minibuffer|review|view/update|math_panels|vcs|
+view-others|update-others
+* \li Origin: daniel, 12 July 2016
+* \endvar
+*/
+               { LFUN_TOOLBAR_MOVABLE, "toolbar-movable", NoBuffer, Buffer },
+/*!
  * \var lyx::FuncCode lyx::LFUN_MENU_OPEN
  * \li Action: Opens the menu given by its name.
  * \li Syntax: menu-open <NAME>
diff --git a/src/frontends/qt4/GuiToolbar.cpp b/src/frontends/qt4/GuiToolbar.cpp
index 77471c9..56dca72 100644
--- a/src/frontends/qt4/GuiToolbar.cpp
+++ b/src/frontends/qt4/GuiToolbar.cpp
@@ -53,7 +53,7 @@ namespace lyx {
 namespace frontend {

 GuiToolbar::GuiToolbar(ToolbarInfo const & tbinfo, GuiView & owner)
-       : QToolBar(toqstr(tbinfo.gui_name), &owner), visibility_(0),
+       : QToolBar(toqstr(tbinfo.gui_name), &owner), visibility_(0), 
movability_(0),

I think this new variable is redundant with isMovable()/setMovable()
from qt and can be made without.

          owner_(owner), command_buffer_(0), tbinfo_(tbinfo), filled_(false),
          restored_(false)
 {
@@ -61,8 +61,6 @@ GuiToolbar::GuiToolbar(ToolbarInfo const & tbinfo, GuiView & 
owner)
        connect(&owner, SIGNAL(iconSizeChanged(QSize)), this,
                SLOT(setIconSize(QSize)));

-       // Toolbar dragging is allowed.
-       setMovable(true);
        // This is used by QMainWindow::restoreState for proper main window 
state
        // restauration.
        setObjectName(toqstr(tbinfo.name));
@@ -111,6 +109,13 @@ void GuiToolbar::setVisibility(int visibility)
 }


+void GuiToolbar::setMovability(bool movability)
+{
+       movability_ = movability;
+}
+
+
+
 Action * GuiToolbar::addItem(ToolbarItem const & item)
 {
        QString text = toqstr(item.label_);
@@ -358,6 +363,7 @@ void GuiToolbar::saveSession() const
 {
        QSettings settings;
        settings.setValue(sessionKey() + "/visibility", visibility_);
+       settings.setValue(sessionKey() + "/movability", movability_);
 }


@@ -374,6 +380,11 @@ void GuiToolbar::restoreSession()
                        
guiApp->toolbars().defaultVisibility(fromqstr(objectName()));
        }
        setVisibility(visibility);
+
+       int movability =
+               settings.value(sessionKey() + "/movability", true).toBool();
+       setMovability(movability);
+       setMovable(movability);

Here you see that movability_ has no real purpose since it just duplicates the movable state.

 }


@@ -409,6 +420,32 @@ void GuiToolbar::toggle()
                qstring_to_ucs4(windowTitle()), state));
 }

+void GuiToolbar::movable(bool silent)
+{
+       // toggle movability
+       setMovability(!isMovable());
+       setMovable(!isMovable());
+
+       // manual repaint avoids bug in qt that the drag handle is not removed
+       // properly, e.g. in windows
+       if (isVisible())
+               repaint();

I am not sure what is the good solution, but what you did seems to work
correctly here.

+
+       // silence for toggling of many toolbars for performance
+       if (!silent) {
+               docstring state;
+               if (isMovable()) {
+                       state = _("movable");
+
+               }
+               else {
+                       state = _("immovable");
+
+               }
+               owner_.message(bformat(_("Toolbar \"%1$s\" state set to %2$s"),
+                       qstring_to_ucs4(windowTitle()), state));
+       }

Ok so this message is only if the user wants to set movability
toolbar-per-toolbar?

I agree that it is simpler to lock all toolbars at once, at least in the
default interface.

+}
 } // namespace frontend
 } // namespace lyx

diff --git a/src/frontends/qt4/GuiToolbar.h b/src/frontends/qt4/GuiToolbar.h
index caad355..4463c06 100644
--- a/src/frontends/qt4/GuiToolbar.h
+++ b/src/frontends/qt4/GuiToolbar.h
@@ -74,6 +74,8 @@ public:

        ///
        void setVisibility(int visibility);
+       ///
+       void setMovability(bool movability);

        /// Add a button to the bar.
        void add(ToolbarItem const & item);
@@ -97,6 +99,9 @@ public:
        ///
        void toggle();

+       /// toggles movability
+       void movable(bool silent = false);
+
        ///
        GuiCommandBuffer * commandBuffer() { return command_buffer_; }

@@ -119,6 +124,8 @@ private:
        QList<Action *> actions_;
        /// initial visibility flags
        int visibility_;
+       /// initial movability flags
+       bool movability_;
        ///
        GuiView & owner_;
        ///
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 1342eb7..2aa7fc7 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -770,16 +770,18 @@ bool GuiView::restoreLayout()

        if (!restoreState(settings.value("layout").toByteArray(), 0))
                initToolbars();
-       
-       // init the toolbars that have not been restored
+
        Toolbars::Infos::iterator cit = guiApp->toolbars().begin();
        Toolbars::Infos::iterator end = guiApp->toolbars().end();
        for (; cit != end; ++cit) {
                GuiToolbar * tb = toolbar(cit->name);
                if (tb && !tb->isRestored())
-                       initToolbar(cit->name);
+                               initToolbar(cit->name);

You have a strange indentation here.


        }

+       // update lock (all) toolbars positions
+       updateLockToolbars();
+
        updateDialogs();
        return true;
 }
@@ -795,6 +797,18 @@ GuiToolbar * GuiView::toolbar(string const & name)
        return 0;
 }

+void GuiView::updateLockToolbars()
+{
+       toolbarsMovable = false;
+       Toolbars::Infos::iterator cit = guiApp->toolbars().begin();
+       Toolbars::Infos::iterator end = guiApp->toolbars().end();
+       for (; cit != end; ++cit) {
+               GuiToolbar * tb = toolbar(cit->name);
+               if (tb && tb->isMovable())
+                       toolbarsMovable = true;
+       }
+}
+

For simple iterations, thanks to new C++ syntax, one will prefer to
write the shorter form that follows:

void GuiView::updateLockToolbars()
{
        toolbarsMovable = false;
        for (ToolbarInfo const & info : guiApp->toolbars()) {
                GuiToolbar * tb = toolbar(info.name);
                if (tb && tb->isMovable())
                        toolbarsMovable = true;
        }
}

(for something more idiomatic one could also use std::any_of which you
can look up if you are curious, but the above is readable enough.)


 void GuiView::constructToolbars()
 {
@@ -863,6 +877,10 @@ void GuiView::initToolbar(string const & name)

        if (visibility & Toolbars::ON)
                tb->setVisible(true);
+
+       // FIXME: shouldn't here be a call of GuiToolbar::movable instead?
+       tb->setMovable(true);
+       tb->setMovability(true);
 }

No, that looks correct because GuiToolbar::movable also deals with
repaint, status bar message etc., which we don't necessarily want here,
and once you remove the movability_ variable there is nothing left to
shorten further.




@@ -1892,6 +1910,28 @@ bool GuiView::getStatus(FuncRequest const & cmd, FuncStatus 
& flag)
                break;
        }

+       case LFUN_TOOLBAR_MOVABLE: {
+               string const name = cmd.getArg(0);
+               // use negation since locked == !movable
+               if (name == "*") {
+                       // toolbar name * locks all toolbars
+                       flag.setOnOff(!toolbarsMovable);
+

There must be a bug here because it is always shown as "on" for me.

+               }
+               else if (GuiToolbar * t = toolbar(name)) {
+                       flag.setOnOff(!(t->isMovable()));
+
+               }
+               else {

I think the style used in the LyX source code has one less line break before and after }.

+                       enable = false;
+                       docstring const msg =
+                               bformat(_("Unknown toolbar \"%1$s\""), 
from_utf8(name));
+                       flag.message(msg);
+
+               }
+               break;
+       }
+
        case LFUN_DROP_LAYOUTS_CHOICE:
                enable = buf != 0;
                break;
@@ -3775,6 +3815,38 @@ void GuiView::dispatch(FuncRequest const & cmd, 
DispatchResult & dr)
                        break;
                }

+               case LFUN_TOOLBAR_MOVABLE: {
+                       string const name = cmd.getArg(0);
+
+                       if (name == "*") {
+                               // toggle (all) toolbars movablility
+                               toolbarsMovable = !toolbarsMovable;
+                               Toolbars::Infos::iterator cit = 
guiApp->toolbars().begin();
+                               Toolbars::Infos::iterator end = 
guiApp->toolbars().end();
+                               for (; cit != end; ++cit) {
+                                       GuiToolbar * tb = toolbar(cit->name);
+                                       if (tb && tb->isMovable() != 
toolbarsMovable) {
+                                               // toggle toolbar movablity if 
it does not fit lock (all) toolbars positions state
+                                               // silent = true, since status 
bar notifications are slow
+                                               tb->movable(true);
+                                       }
+                               }
+                               if (toolbarsMovable) {
+                                       dr.setMessage(_("All toolbars 
unlocked."));
+                               }
+                               else {
+                                       dr.setMessage(_("All toolbars 
locked."));
+                               }
+                       }
+                       else if (GuiToolbar * t = toolbar(name)) {
+                               // toggle current toolbar movablity
+                               t->movable();
+                               // update lock (all) toolbars positions
+                               updateLockToolbars();
+                       }
+                       break;
+               }
+
                case LFUN_DIALOG_UPDATE: {
                        string const name = to_utf8(cmd.argument());
                        if (name == "prefs" || name == "document")
diff --git a/src/frontends/qt4/GuiView.h b/src/frontends/qt4/GuiView.h
index 33bfd97..8f58b6b 100644
--- a/src/frontends/qt4/GuiView.h
+++ b/src/frontends/qt4/GuiView.h
@@ -210,6 +210,9 @@ public:
        /// Current ratio between physical pixels and device-independent pixels
        double pixelRatio() const;

+       // movability flag of all toolbars
+       bool toolbarsMovable;

Actually I wonder whether it's useful to store the flag and update it
by hand, since it is not very expensive to compute it every time. Having
this flag looks like a premature optimisation.

+
 Q_SIGNALS:
        void closing(int);
        void triggerShowDialog(QString const & qname, QString const & qdata, 
Inset * inset);
@@ -357,6 +360,8 @@ private:
        void initToolbars();
        ///
        void initToolbar(std::string const & name);
+       /// Update lock (all) toolbars position
+       void updateLockToolbars();
        ///
        bool lfunUiToggle(std::string const & ui_component);
        ///
diff --git a/src/frontends/qt4/Menus.cpp b/src/frontends/qt4/Menus.cpp
index 0d80b56..10aba3b 100644
--- a/src/frontends/qt4/Menus.cpp
+++ b/src/frontends/qt4/Menus.cpp
@@ -1340,6 +1340,7 @@ void MenuDefinition::expandToc(Buffer const * buf)
                item.setSubmenu(other_lists);
                add(item);
        }
+
        // Handle normal TOC
        add(MenuItem(MenuItem::Separator));
        cit = toc_list.find("tableofcontents");
@@ -1389,6 +1390,12 @@ void MenuDefinition::expandToolbars()
                item.setSubmenu(other_lists);
                add(item);
        }
+
+       add(MenuItem(MenuItem::Separator));
+
+       add(MenuItem(MenuItem::Command, qt_("Lock Toolbars Positions|L"),
+               FuncRequest(LFUN_TOOLBAR_MOVABLE, "*")/*, "toolbar-movable 
*"*/));

extra comment

+
 }


All in all it seems to be a really good start for a patch, with some
opportunities for simplification and only one bug mentioned above.

Guillaume

Reply via email to