Hi Guillaume,
Thanks a lot. I have added some comments. I tried to fix what I could. I
am a total amateur in this. I don't know what bug there should be since
it works as expected for me. It would be great if you give me further
instructions or just show me how to do the improvements. While I think
it is helpful to do exercises to learn it myself, I also need to see a
couple of times how to do it first.
On 21.09.2016 17:24, Guillaume Munch wrote:
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.)
Well, I have to figure out how to do a proper patch then. (I just used
git gui's "Make patch" function (Repository > Visualize master's
History, right click on "Local changes checked in to the index but not
committed" and choose "Make patch"). I also entered my name in git gui
under Options. But that's about it.)
* 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 have now I think.
* I expected to find the lock toolbar option when right-clicking on the
toolbars. Do you know where to add this?
Yes, I do. But I think it should be like this:
1. There should be a LFUN for setting the icon size.
2. The four default icon sizes from the widget (LyX main window) context
menu should go to the Toolbars menu.
3. The whole Toolbars menu should appear when the widget's context menu
is opened.
Some more specific comments below.
Me too.
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.
I just did a copy of the visibility for movability. I am also not sure
how to replace its functionality.
So does this imply that that variable is redundant too?
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.
I used the defaults tabs in visual studio. I think there is an option
called "Smart" indentation. Maybe it is not so smart after all to use
it. I have set it now to block. Now I have to manually start new
indentation but at least they are now all 4 spaces wide.
}
+ // 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.
I don't understand what is always "on" here. The "Lock Toolbars
Positions" menu entry does get checked and unchecked, right?
+ }
+ 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 }.
I have seen it with and without the break in the code. But I removed it now.
+ 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.
I thought it is needed for the "Lock Toolbars Positions". Don't know how
to do without.
+
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
If I remember correctly, the comment will set a message on the status
bar. But I guess LyX has it's own way to do it.
+
}
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.
Daniel
