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