Reviewing merge request #1915: Allow positioning toolbars and dock widgets via API

Started out as a support request, got assigned a task tracker ID (247052) but no implementation schedule so we desided to give it a go ourselves. The patch allows you to position tool bars and dock widgets via their standard widget methods (move(), setGeometry() etc.) and have QMainWindow synchronize its internal layout structures from this geometry information (as long as the event loop isn't triggered in between). Tested it on WIN32 and works as required; unfortunately we don't have other build platforms available.

Commits that would be merged:

Version 1
  • Version 1
  • 511c434
  • 570fe10
  • Added functionality to position toolbars and dock widgets programmatically via synchronizeFromToolBars() and synchronizeFromDockWidgets(), see TT247052

Showing 511c434-570fe10

Comments

Wouldn’t it make more sense to ask the main window to resize/move a widget?
It seems the main window is in charge and having this synchronization seems to be there to have a backdoor to this fact.

That would be a possibility, however with a direct approach you'd have to trigger the layout functions for each widget you touch. Maybe the overhead isn’t so bad, but for something like our application where we’ll have to position something like 20 or 30 toolbars and docks I'd rather do it in bulk.

How about this: we move the synchronize-methods to the private sections and add a helper class QToolDockPositioner or something as friend of QMainWindow (or its private data pointer, I don’t know by heart) which has the calls to position toolbars and dockwidgets and calls the necessary synchronize-methods in its destructor. Internally I'd rather not change the basic approach of using the toolbar/dock widget itself to transport the desired geometry, though.

I haven’t heard from anyone since my last suggestions. Since 4.6 is out now developers should have a bit of time at last to give me some feedback. I don’t want to start implementing what I suggested unless I know I'm moving in the right direction.

I haven’t tried it myself but I would still prefer a way to move/resize the toolbars directly from the mainwindow. The layout doesn’t have to be done each time: there is a posted LayoutRequest event in the case a layout changes.

I've given this some thought and I see the following problems:
The LayoutRequest event does something different: it recalculates its internal layout items, then overwrites the widget positions with this geometry. Currently I do it the other way around, so I'd either need a new event (ugly and unreliable since it'd be in direct rivalry to LayoutRequest) or change how I do it. If I changed it, I'd either have to work on the internal layout items for every repositioning request (and thus redo the layout every time) or store a list of pending geometry change requests (basically (item, geometry)-tuples) and evaluate those first upon receiving a LayoutRequest. I really don’t like the first one and the second one would have an impact on the actual data structures (only private pointers, but nevertheless). I also don’t know how well this would integrate with regular LayoutRequest processing, but that’s hopefully a minor issue. I'm willing to give it a shot, but since it’s a rather big change I think it should be the final design, so please let me know whether this approach (in particular storing pending geometry change requests in the private pointers) is OK with you.

→ State changed from New to Revise and resubmit

I would expect some new public API on the QMainWindow class (as that is the one that owns and manages the dockers and toolbars). The way its done in this patch makes it really hard to discover the feature and probably confusing as thierry pointed out because it splits the responsibility of positioning between two classes.

Advantage of adding API on QMainWindow is also that you can have a way to do something like “position toolbar X before toolbar Y” which will be awkward with calling ‘move’ on the toolbar itself. Each application would need to implement its own layout to determine the positions. I think that should not be what Qt suggests.

I'd love to have a way to allow positioning dock widgets using C++ API, but we need good API to merge it into Qt.

Alright, how about this:
I add a new helper class, something like QToolDockMover which is bound to a QMainWindow in its constructor. This class gets methods for moving and resizing QToolBars and QDockWidgets, which essentially just change the widget geometry. In its destructor (or an optional flush() method) it then triggers the synchronizeFrom() methods I implemented in the private pointer of QMainWindow; the synchronizeFrom()-methods in the QMainWindow API are removed.
Advantages: you get your explicit API, while much of the current implementation can remain as is, since it’s hidden from the public interface anyway. No additional means of transporting geometry information is required. And because the actual layout isn’t performed immediately by default but normally in the helper class' destructor, you can efficiently do the whole toolbar area(s) in one go.
If that design is OK with you, I can implement it and resubmit. I won’t embark on a wild goose change, however, so until I get an answer, nothing will happen. If you’re OK with the design, let me know what names you'd prefer for the helper class and whether it should go into a new header or whether I can just add it to QMainWindow (it’ll be a tiny class anyway, probably just 5 methods).

→ State changed from Revise and resubmit to Closed

Thank you for your contribution. With Qt 4 having moved [1] under Open
Governance [2] as well, you can now contribute your changes via Gerrit
[3]. At the same time, contributions via Gitorious can no longer be
merged.

We would still like to see your patch on the new platform, and wish
for you to become a member of the Qt Project. Please read [4] and in
particular [5] to familiarize yourself with the new tools. You will
still find a read-only mirror of all Qt repositories on Gitorious.

With a much wider set of possible reviewers, we are also looking
forward to better response times to contributions. Looking forward to
your patch!

Please note that at this point in time Qt 4.8 is a bugfix only branch,
and all fixes should be submitted to Qt 5 first, as far as
applicable. Qt 4 master is closed.

The Nokia Qt team

[1] http://labs.qt.nokia.com/2012/01/10/qt-4-moved-to-open-governance/
[2] http://labs.qt.nokia.com/2011/10/21/the-qt-project-is-live/
[3] http://codereview.qt-project.org
[4] http://www.qt-project.org
[5] http://developer.qt.nokia.com/wiki/Gerrit_Introduction

Add a new comment:

Login or create an account to post a comment

How to apply this merge request to your repository