Le 22/09/2016 à 09:18, racoon a écrit :
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 :
* 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.

I see what you mean. It really depends on how much effort you are ready to put into this. It might be overkill.

 GuiToolbar::GuiToolbar(ToolbarInfo const & tbinfo, GuiView & owner)
-    : QToolBar(toqstr(tbinfo.gui_name), &owner), visibility_(0),
+    : QToolBar(toqstr(tbinfo.gui_name), &owner), visibility_(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.

GuiToolbar inherits QToolBar::isMovable() whose value happens to
coincide with movability_ in your patch.

So does this imply that that variable is redundant too?

I quickly checked and as I can see that in GuiView::initToolbar,
setVisible and setVisibility are called differently, so I cannot
conclude that visibility_ is redundant.

         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.

Ideally one wants automatic indentation, but I could only help you with

+        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?

It's always checked for me, I don't really know why. But it correctly switches movable status.

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.

Instead of a member variable bool toolbarsMovable you could have a function bool toolbarsMovable() const, that would compute the value as you did previously.


Reply via email to