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
