Gui: support dragging of toolbar inside status/menu bar #13973

realthunder · 2024-05-12T08:21:42Z

Continuation of PR #13571. @PaddleStroke

kadet1090 · 2024-05-12T11:48:55Z

src/Gui/ToolBarManager.cpp

    }
 }

+void ToolBarManager::setToolBarMovable(QToolBar *toolbar) const


Since you already added custom ToolBar class, would it be vise to move that logic there? Having it separated can cause a lot of confusion when you don't know that you have to call this function.

I wonder if it would not be better to make this function as handler for movableChanged signal of QToolBar: https://doc.qt.io/qt-6/qtoolbar.html#movableChanged.

At least it should have more descriptive name because for the whole review process I was sure that it calls setMovable() in the toolbar while it does not.

kadet1090 · 2024-05-12T11:53:10Z

src/Gui/ToolBarManager.cpp

@@ -756,6 +893,18 @@ void ToolBarManager::restoreState() const
    menuBarLeftArea->restoreState(mbLeftToolBars);
 }

+ToolBarArea *ToolBarManager::getToolBarArea(QToolBar *toolbar) const


In #13721 I already have such method, but it does more like Qt does it so via enum. This method also covers standard Qt::ToolBarArea's: https://github.com/FreeCAD/FreeCAD/pull/13721/files#diff-dafe6a032a9dc3c2149c60fc7319fbdcda9b32e255a21185f62849cc8ba41902R438. Can you rework your code to be more inline with it so we won't have conflicts?

kadet1090 · 2024-05-12T11:54:18Z

src/Gui/ToolBarManager.cpp

+    {
+        QSignalBlocker blocker(toolbar);
+        area->removeWidget(toolbar);
+        getMainWindow()->addToolBar(toolbar);
+        toolbar->setWindowFlags(Qt::Tool
+                | Qt::FramelessWindowHint
+                | Qt::X11BypassWindowManagerHint);
+        toolbar->adjustSize();
+        pos = toolbar->mapFromGlobal(pos);
+        toolbar->move(pos.x()-10, pos.y()-10);
+        toolbar->setVisible(true);
+    }
+    toolbar->topLevelChanged(true);


Since you already have custom ToolBar class it should be some method of that class, like void float().

kadet1090 · 2024-05-12T11:56:08Z

src/Gui/ToolBarManager.cpp

+        painter.setPen(Qt::transparent);
+        painter.setOpacity(0.5);
+        painter.setBrush(QBrush(Qt::black, Qt::Dense6Pattern));
+        QRect rect(this->rect());
+        painter.drawRect(rect);


Why is drawing of this widget different when it is not in the custom toolbar area?

kadet1090 · 2024-05-12T11:56:18Z

src/Gui/ToolBarManager.cpp

+void ToolBarGrip::paintEvent(QPaintEvent*)
+{
+    QPainter painter(this);
+    if (auto toolbar = qobject_cast<QToolBar*>(parentWidget())) {


Why not ToolBar*? You do static_cast after it anyway so you assume that it in fact is the ToolBar*.

kadet1090 · 2024-05-12T12:04:13Z

src/Gui/ToolBarManager.cpp

+    if (!toolbar)
+        return;


I know that this classes do not have automatic code formatting enabled - but it would be nice to adhere to the formatting requested by the project.

kadet1090 · 2024-05-12T12:05:42Z

src/Gui/ToolBarManager.cpp

+    if (sep) {
+        if (movable) {


why not if (sep && movable) ?

kadet1090 · 2024-05-12T12:07:12Z

src/Gui/ToolBarManager.cpp

+        area->adjustParent();
+    }
+    QString name = QStringLiteral("_fc_toolbar_sep_");
+    auto sep = toolbar->findChild<QAction*>(name);


auto separator - let's not use such short names, it makes code harder to read and understand.

kadet1090 · 2024-05-12T12:08:46Z

src/Gui/ToolBarManager.cpp

+            }
+        }
+        else {
+            new ToolBarGrip(toolbar);


Creating new object without assigning it to anything is a very strange construction. Probably it is fine as it has properly defined parent so Qt will take care of it but nonetheless it is quite odd - please at least put comment over it that it is intentional and how it is supposed to work.

kadet1090 · 2024-05-12T12:10:46Z

src/Gui/ToolBarManager.cpp

+        toolbar->move(pos.x()-10, pos.y()-10);
+        toolbar->setVisible(true);
+    }
+    toolbar->topLevelChanged(true);


While you don't have to mark signals with explicit Q_EMIT it is nice at it clearly states the intention of that call. Also a comment why it is outside of the block above (to not block this signal - a bug that you have in the previous PR) would be nice as it once again is hard to quickly understand why every call is in that block and it is outside.

PaddleStroke · 2024-05-13T08:22:15Z

@realthunder thanks !
Btw have you seen the bug report by Syres in the previous PR? : https://forum.freecad.org/viewtopic.php?p=758767#p758767

Is this issue fixed in this PR?

PaddleStroke · 2024-05-13T09:05:37Z

I have tested the functionality and it works great. Much better than the previous 'unlock toolbar' menu.

One small note :
Before with the 'Corner widget' option, the tabbar was added 'Raw' as the corner widget. Resulting in no margin on top/bottom.
Since #13571, the tabbar is now inside a toolbar, and it's the toolbar that is put as corner widget, resulting in some margins :

I don't know if it is possible, but if toolbars that are in the menubar/statusbar could have no top/bottom margins it would look better.

PaddleStroke · 2024-05-13T11:56:17Z

Also I think that you may have introduced the issue #13927 when fixing the sketcher toolbar problem in #13571

I have not bisected so I'm not 100% sure but I have a build from before and one after your PR, and it the bug is only in the build after.

I would guess that a preference is not initialized correctly?

FEA-eng · 2024-05-20T10:48:20Z

@realthunder Can you have a look at the proposed changes? Also, there are some conflicts that have to be resolved.

PaddleStroke · 2024-05-20T12:50:58Z

Yes it would be really good if this can get in before 1.0 because it's really complementing #13571 making things more natural.

kadet1090 · 2024-05-20T12:56:10Z

Not in that state. This is another PR that will require a lot of refactoring after merging it.

FEA-eng · 2024-05-20T13:16:51Z

Not in that state. This is another PR that will require a lot of refactoring after merging it.

Hence the requested changes. Do you have more?

kadet1090 · 2024-05-20T13:27:39Z

Hence the requested changes. Do you have more?

Yes and no, depends on how you look at it. I did comment on the stuff that is most important and in my opinion should be changed or clarified before merging so in that way no. But I have at least twice as much questions to the PR that are less important and I decided to simply not flood this PR with comments.

FEA-eng · 2024-05-20T13:34:22Z

Ok, we have to wait for Realthunder’s feedback anyway.

PaddleStroke · 2024-05-20T13:59:54Z

Alternatively, if you already knows what needs to be done (as it appears in the review you made) perhaps we can merge it and you can PR the necessary fixes @kadet1090 ?

FEA-eng · 2024-05-20T14:12:36Z

@PaddleStroke As long as the PR is open and non-draft, it should be considered for merging even after the freeze date.

realthunder · 2024-05-20T14:13:30Z

I am a bit busy these few days. Feel free to change the code as you like.

kadet1090 · 2024-05-20T14:21:24Z

Alternatively, if you already knows what needs to be done (as it appears in the review you made) perhaps we can merge it and you can PR the necessary fixes @kadet1090 ?

I'd rather create another PR to replace this one with fixes included to not risk that it will stay like this and fight with potential conflicts that can happen due to others fixing the same piece of code (which happened with #13571). That said, I also won't have time to do that until next saturday - after that I should have more time for FreeCAD.

Let's keep this open for now, like it was said - it will get merged as this is marked as ready for review so it can be merged even after the feature freeze.

Gui: support dragging of toolbar inside status/menu bar

4f12501

github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label May 12, 2024

kadet1090 suggested changes May 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gui: support dragging of toolbar inside status/menu bar #13973

Gui: support dragging of toolbar inside status/menu bar #13973

realthunder commented May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

kadet1090 May 12, 2024

PaddleStroke commented May 13, 2024 •

edited

PaddleStroke commented May 13, 2024

PaddleStroke commented May 13, 2024 •

edited

FEA-eng commented May 20, 2024

PaddleStroke commented May 20, 2024

kadet1090 commented May 20, 2024

FEA-eng commented May 20, 2024

kadet1090 commented May 20, 2024

FEA-eng commented May 20, 2024

PaddleStroke commented May 20, 2024

FEA-eng commented May 20, 2024

realthunder commented May 20, 2024

kadet1090 commented May 20, 2024

Gui: support dragging of toolbar inside status/menu bar #13973

Are you sure you want to change the base?

Gui: support dragging of toolbar inside status/menu bar #13973

Conversation

realthunder commented May 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaddleStroke commented May 13, 2024 • edited

PaddleStroke commented May 13, 2024

PaddleStroke commented May 13, 2024 • edited

FEA-eng commented May 20, 2024

PaddleStroke commented May 20, 2024

kadet1090 commented May 20, 2024

FEA-eng commented May 20, 2024

kadet1090 commented May 20, 2024

FEA-eng commented May 20, 2024

PaddleStroke commented May 20, 2024

FEA-eng commented May 20, 2024

realthunder commented May 20, 2024

kadet1090 commented May 20, 2024

PaddleStroke commented May 13, 2024 •

edited

PaddleStroke commented May 13, 2024 •

edited