Dear Qt friends, hackers and freaks, recently I've noticed a refactoring changes regarding QAction and QActionGroup, happening inside "dev" branch of Qt (https://codereview.qt-project.org/c/qt/qtbase/+/265909). I didn't have a chance to review it before it got merged, I just missed it. So I took a liberty to look what we are going to have in Qt 6 and reviewed it "post factum".
I need to admit, that this is a very good change! The main goal, which is moving QAction out of Widget lib and placing it in some more general lib in order to use it not only in Widget world, is a really good choice! Good work Friedemann and all reviewers! However, when we say A, I'm very tempted to say also B (or even C), so why not to take one or two more steps with it? =============================================== The current state in short ("dev" branch of Qt) =============================================== The QAction (and QActionGroup) has been split into two classes: - QGuiAction (and QGuiActionGroup) is the base, lives outside of Widget lib, contains most of the API of old QAction (QActionGroup) except the API referring to Widget lib, like methods containing pointers to QMenu, QWidget. - QAction (and QActionGroup) is derived from QGuiAction (QGuiActionGroup), enriching the base QGuiAction (QAGuictionGroup) a little bit, lives in old place inside Widget lib At a quick glance: class QAction : public QGuiAction { ... QMenu *menu() const; void setMenu(QMenu *menu); bool showStatusText(QWidget *widget = nullptr); QWidget *parentWidget() const; QList<QWidget *> associatedWidgets() const; QList<QGraphicsWidget *> associatedGraphicsWidgets() const; }; And nothing really more than the above. So, QAction is now a small decoration, enriching the QGuiAction with methods referring to QWidget world. =============================== The rationale for further steps =============================== While I think the very first step, very good step, has already been done, I believe we may do something more to make people more happy. With the current state users might start wondering: - There are (will be) libs which instatiate QGuiActions and QActions. How then I use them together in one app? E.g. I've received pointer to QGuiAction let's say from a lib which implements QML bindings (so I receive an action from QML world). Can I put this action into the QMenu directly? Probably I need to duplicate the QGuiAction by creating the respective QAction, keeping them in sync on some map and put the QAction to the QMenu. - I'm writing my lib (not linked against QtWidgets), and I want to expose actions through my API. Should I use QGuiAction in the API? Should I instantiate the QGuiAction in my lib? But then the other lib, which is dependent on QtWidgets won't be able to use actions returned by my API. - I'm probably going to introduce quite a lot downcasts in my code nowadays, like q-object_cast<QAction *>(q_gui_action_pointer) since parts of Qt API are going to use QGuiAction, while other parts are going to use QAction. That's why I believe, that having just one class (QAction) instead of two (QGuiAction, QAction) would solve all the above concerns. ======================================================== The next step: keep one class, enrich not by inheritance ======================================================== Being a fan of a principle "prefer aggregation over inheritance", I would like to propose the following changes to the current state: 1. Rename QGuiAction (QGuiActionGroup) back to QAction (QActionGroup) without any other changes to its current API. 2. Remove current QAction (QActionGroup) subclasses (which currently live in QtWidgets lib) and don't introduce any new class for it. We than need to figure out what to do with the API which currently is there, and how to enrich existing QAction, not by inheritance. 3. For the existing QAction's API I propose to move the respective functions to the other classes involved (e.g. as static methods), like: QMenu *menu() const; -> static QMenu *QMenu::menuForAction(QAction *action); void setMenu(QMenu *menu); -> static void QMenu::setMenuForAction(QAction *action, QMenu *menu); bool showStatusText(QWidget *widget = nullptr); -> static bool QWidget::showStatusText(QAction *action, QWidget *widget = nullptr); QWidget *parentWidget() const; -> static QWidget *QWidget::parentWidget(QAction *action); QList<QWidget *> associatedWidgets() const; -> static QList<QWidget *> QWidget::associatedWidgets(QAction *action); QList<QGraphicsWidget *> associatedGraphicsWidgets() const; -> static QList<QGraphicsWidget *> QGraphicsWidget::associatedGraphicsWidgets(QAction *action); 4. For the QActionGroup it looks like all functions may just disappear, as they are kind of repetition of functions from QGuiActionGroup. Regarding triggered() and hovered() signals: we may move them back to the base class, as currently we have similar signals in the action base class. ============= Further steps ============= 1. I would like to ask all of you what do you think about the above proposal? It would be really cool if you could state whether you like the proposed direction or not. 2. In case we agree if we want further changes, I invite you for brainstorming on proposed names for functions moved out of current QAction, their location (contained in right class or the other class would be better) and their "staticness". We may also discuss other possibilities on how to handle the remaining API of current QAction / QActionGroup. 3. In case we develop together the API we want, we could start discussing the implementation details, like where to put the enriched internals. Qt is very comprehensive on that, so for sure we will have couple of choices there. I would very much like to try this API-driven (not implementation-driven) approach when doing API changes. Like in old Jasmin's time: we need to first develop the API we would like to have and use, and later start figuring out implementation challenges. So, discussing point 2 or 3, without having agreement on 1 wouldn't be much useful I guess. Big thanks and regards Jarek _______________________________________________ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development