How to recover tabOrder after switching `windowFlags` of some widgets in Qt?
Asked Answered
L

2

6

Context:

For a custom widget, similar to a menu, I have some of the content inside a "drop-down" frame when space is not sufficient:

enter image description here

For this drop-down feature, I set the frame for those buttons to have frame.setWindowFlags(Qt::Popup | Qt::FramelessWindowHint);. This works pretty well, except that it mess the tabulation order.

The structure of widgets look like:

MenuWidget
 |- Tab1
     |- Group_<without_name>
         ...
     |- Group_File
         ...
     |- Group_Export
         |-Frame (can be either standard sub-widget, or a popup widgets)
             |- Button 1
             |- Button 2
                 ....

You can see the current WIP there: https://github.com/Escain/QTopMenu


I reduced the issue to the following code, which has proper tabulation order at the begining, and has messed tabulation after I set Qt::Popup and back to Qt::Widget. Any attempt to recover the proper tabulation order is failing.

Before setting Qt::Popup:

enter image description here

After setting Qt::Popup and setting back Qt::Widget:

enter image description here


Here is the minimal example to reproduce the issue.
DISCLAIM: this is not production code, just quick-done code aiming to reproduce the same issue.

#include <QApplication>
#include <QDebug>
#include <QWidget>
#include <QPushButton>

class TestWidget: public QWidget
{
    Q_OBJECT
public:
    explicit TestWidget( QWidget* parent=nullptr)
        : QWidget(parent)
        , frame(this)
        , but1(&frame)
        , but2(&frame)
    {
        but1.setGeometry(10, 10, 80, 30);
        but2.setGeometry(110, 10, 80, 30);
        frame.setGeometry(0,0,200,50);
        resize(200,50);
        setStyleSheet("background-color: rgba(0,128,128,32);");

        setFocusProxy(&frame);
        frame.setFocusProxy(&but1);
        setFocusPolicy(Qt::FocusPolicy::StrongFocus);

        connect(&but1, &QPushButton::clicked, [this]()
        {
            const auto flag = frame.windowFlags();

            // In some circumstances, the widget needs to show it content as drop-down
            frame.setWindowFlags(Qt::Popup | Qt::FramelessWindowHint);

            //try to recover here
            frame.setWindowFlags(flag);
            frame.show();
            emit needsTabOrder();
        });
    }

    QWidget* orderTabs( QWidget* first)
    {
        setTabOrder(first, &but1);
        setTabOrder(&but1, &but2);
        return &but2;
    }

signals:
    void needsTabOrder();
private:

    QWidget frame;
    QPushButton but1, but2;
};
#include "main.moc"


auto main (int argn, char* args[])-> int
{
    QApplication app(argn, args);

    QWidget window;

    TestWidget t1(&window);
    TestWidget t3(&window); //Intentionally out of order
    TestWidget t2(&window);
    TestWidget t4(&window);
    t1.move(0,10);
    t2.move(200,10);
    t3.move(400,10);
    t4.move(600,10);

    QWidget::setTabOrder(&t1, &t2);
    QWidget::setTabOrder(&t2, &t3);
    QWidget::setTabOrder(&t3, &t4);

    auto tryRecoverThisMess = [&t1, &t2, &t3, &t4]()
    {
        qDebug() << "Trying to recover this mess";

        // Attempt 1: not working
        QWidget::setTabOrder(&t1, &t2);
        QWidget::setTabOrder(&t2, &t3);
        QWidget::setTabOrder(&t3, &t4);

        // Attempt 2: very hacky and not working
        QWidget* w=nullptr;
        w = t1.orderTabs( w);
        w = t2.orderTabs( w);
        w = t3.orderTabs( w);
        w = t4.orderTabs( w);
    };

    QWidget::connect(&t1, &TestWidget::needsTabOrder, tryRecoverThisMess);
    QWidget::connect(&t2, &TestWidget::needsTabOrder, tryRecoverThisMess);
    QWidget::connect(&t3, &TestWidget::needsTabOrder, tryRecoverThisMess);
    QWidget::connect(&t4, &TestWidget::needsTabOrder, tryRecoverThisMess);

    window.resize(800,200);
    window.show();

    return app.exec();
}

Using Qt 5.15.2

The CMakeLists looks like:

cmake_minimum_required(VERSION 3.13)
cmake_policy( SET CMP0076 NEW )

project("QtTest")

set(CMAKE_AUTOMOC ON)

find_package(Qt5Widgets REQUIRED)
include_directories(${Qt5Widgets_INCLUDE_DIRS})
add_definitions(${Qt5Widgets_DEFINITIONS})
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${Qt5Widgets_EXECUTABLE_COMPILE_FLAGS}")


set(TARGET "QtTest")
add_executable(${TARGET})

target_compile_options( ${TARGET} PRIVATE -Wall -pedantic -Wno-padded -Werror=return-type)
target_compile_features( ${TARGET} PUBLIC cxx_std_17)

target_sources( ${TARGET} PRIVATE "main.cpp")
target_link_libraries(${TARGET} "Qt5::Widgets" )

My attempts to solve the issue:

  1. I tried to call setTabOrder on the parent widgets, as well as on each button (hacky). Without success.

  2. To print the tab-order, it indeed does NOT re-order as per setTabOrder

  3. Different setWindowFlags values

  4. Several combinations of setFocusProxy and setFocusPolicy

  5. Having the frame always as "Popup" and move all the widgets on it between the frame and the main widget.

The question:

How to recover the tab order after setting Qt::Popup flag?

Limiter answered 4/12, 2021 at 14:59 Comment(2)
Please include the .pro file as wellTruthvalue
I added the CMakeLists.txtLimiter
L
0

The short answer: basically you can't fix a widgets tab order automagically. Setting a QWidget as popup removes it from the QWidget tab order hierarchy because a popup is kind of a window with its very own internal tab hierarchy. By resetting your popup back to a simple widget it is then readded to the tab hierarchy at the and of the level you added it to scrambling everything inbetween. This seems to be a problem of the Qt library. I guess your idea is the only way to recover the taborder afterwards. Related thread about readding widgets. (Your Attempt 2 is basically what is suggested there)

You can verify how messed up your tab order becomes by calling this function after you tried to fix it with your very sensible Attempt 1 inside the main-function-lambda:

void printTabOrder(QWidget *start)
{
    QWidget *current = start;

    do
    {
        qDebug() << current;
        current = current->nextInFocusChain();
    } while(current != start);
}

I would advice you not to change the widgets window flags on the fly. Insted just create a customly behaving and styled QWidget as a popup. IMHO it would be better style anyway because your TestWidget is just the thing holding the buttons and handling clicks on them and your TestWidgetMenu could handle all the popup stuff.

I attribute all structural and style issues to the minimal-working-exaple-nature of this code.

Lustihood answered 12/12, 2021 at 11:10 Comment(0)
T
-1
  1. Never use auto

  2. Only use Lambda's at gun point

    auto tryRecoverThisMess = [&window, &t1, &t2, &t3, &t4]()
    {
        qDebug() << "Trying to recover this mess";
    
        // Attempt 1: not working
        window.setTabOrder( &t1, &t2 );
        window.setTabOrder( &t2, &t3 );
        window.setTabOrder( &t3, &t4 );
    

    #if 0 // Attempt 2: very hacky and not working QWidget *w=nullptr; w = t1.orderTabs( w ); w = t2.orderTabs( w ); w = t3.orderTabs( w ); w = t4.orderTabs( w ); #endif

    };
    

Had you created a class MyClass derived from QWidget and given sad class a slot void tryRecoverThisMess();

You would not have needed parameters because TestWidget items would have been member variables. More importantly the slot would only need.

setTabOrder( &t1, &t2 );
setTabOrder( &t2, &t3 );
setTabOrder( &t3, &t4 );

because it would be executing within the widget.

You are calling QTabWidget::setTabOrder inside a Lambda with an auto type. The compiler was free to either determine this Lambda was a QWidget and set the tab order for it, or to manufacture a temporary QWidget setting its tab order before disposing of it at the end of the Lambda's life.

There was nothing in your code identifying that you wanted to change the tab order of "window."

Admittedly I tried this on Ubuntu 20.04 LTS using Qt 5.12.8, but I did see things jump to the third button after the click. After my fix they didn't do that. The "correct" fix would be to put this stuff in a class and connect the SIGNAL to a slot in the class.

I suspect you have similar parenting issues in your other code.

Restructure you code to delete all Lambda's and remove all autos.

Lambda's as slots are really dangerous. They get extremely dangerous when it comes to objects created then moved to another thread. The Lambda isn't part of the object, it didn't have to move. That means you could have a queued connection and the object that this Lambda slot is trying to return to could be deleted.

Young developers like to use auto and Lambda because it lets them take shortcuts.

The longest distance between any two points is a shortcut.

When you get older you will realize that's a fact in IT.

Welcome to the wonderful world of programming!

Per your request I "patched" your code. It needs to be redesigned from scratch to be "fixed."

#include <QApplication>
#include <QDebug>
#include <QWidget>
#include <QPushButton>

class TestWidget: public QWidget
{
    Q_OBJECT
public:
    explicit TestWidget( QString txt, QWidget *parent=nullptr )
        : QWidget( parent )
        , button1( nullptr )
        , button2( nullptr )
    {
        // don't create widgets on the stack
        //
        QString lbl1 = QString( "%1-%2" ).arg( txt ).arg( 1 );
        QString lbl2 = QString( "%1-%2" ).arg( txt ).arg( 2 );
        button1 = new QPushButton( lbl1, this );
        button2 = new QPushButton( lbl2, this );

        button1->setGeometry( 10, 10, 80, 30 );
        button2->setGeometry( 110, 10, 80, 30 );

        setGeometry( 0,0,200,50 );

        resize( 200,50 );  // ????

        setStyleSheet( "QPushButton{ background-color: rgba(170, 176, 81, 200); }"
                       "QPushButton:focus{ background-color: rgba(72, 75, 224, 200); } "
                       "QPushButton:focus:pressed{ background-color: rgba(229, 141, 29, 200); } "
                       "QPushButton:pressed{ background-color: rgba(241, 5, 10, 250); } " );

        setFocusProxy( this );
        setFocusProxy( button1 );
        setFocusPolicy( Qt::FocusPolicy::StrongFocus );

        setInternalTabOrder();
        connect( button1, &QPushButton::clicked, [this]()
        {
            const auto flag = this->windowFlags();

            // In some circumstances, the widget needs to show it content as drop-down
            this->setWindowFlags( Qt::Popup | Qt::FramelessWindowHint );

            //try to recover here
            this->setWindowFlags( flag );
            this->show();
            emit needsTabOrder();
            this->setFocus( );  // without this clicked button won't grab
            // focus but a clicked button2 will
        } );
    }

    ~TestWidget()
    {
        if ( button1 != nullptr )
        {
            delete button1;
            button1 = nullptr;
        }

        if ( button2 != nullptr )
        {
            delete button2;
            button2 = nullptr;
        }
    }

    void setInternalTabOrder()
    {
        setTabOrder( button1, button2 );
    }
signals:
    void needsTabOrder();
private:

    QPushButton *button1;
    QPushButton *button2;
};
#include "main.moc"


auto main ( int argn, char *args[] )-> int
{
    QApplication app( argn, args );

    // don't create widgets on the stack
    //
    QWidget *window = new QWidget();

    TestWidget *t1 = new TestWidget( "Fred", window );
    TestWidget *t3 = new TestWidget( "Ethyl", window ); //Intentionally out of order
    TestWidget *t2 = new TestWidget( "Lucy", window );
    TestWidget *t4 = new TestWidget( "Ricky", window );

    t1->move( 0,10 );
    t2->move( 200,10 );
    t3->move( 400,10 );
    t4->move( 600,10 );

    QWidget::setTabOrder( t1, t2 );
    QWidget::setTabOrder( t2, t3 );
    QWidget::setTabOrder( t3, t4 );

    auto tryRecoverThisMess = [window, t1, t2, t3, t4]()
    {
        qDebug() << "Trying to recover this mess";

        window->setTabOrder( t1, t2 );
        window->setTabOrder( t2, t3 );
        window->setTabOrder( t3, t4 );

    };

    QWidget::connect( t1, &TestWidget::needsTabOrder, tryRecoverThisMess );
    QWidget::connect( t2, &TestWidget::needsTabOrder, tryRecoverThisMess );
    QWidget::connect( t3, &TestWidget::needsTabOrder, tryRecoverThisMess );
    QWidget::connect( t4, &TestWidget::needsTabOrder, tryRecoverThisMess );

    window->resize( 800,200 );
    window->show();

    return app.exec();
}

Despite all of the down votes from haters who want to use auto and lambda everywhere, this is the only working code that has been posted, and it has been tested. Nobody else took the time to actually write working code.

Truthvalue answered 11/12, 2021 at 22:32 Comment(12)
I made the changes you are suggesting, and still have the same issue. Could you share your modification?Limiter
I only fixed one of the problems, the one you asked about. This code has many problems. I will see if I can work on it this morning.Truthvalue
As to the "siths" comment, we don't really condone name calling on SO. When one works in the medical device world they work in absolutes. It's called Deterministic Programming. You have a shining example of how one messes up with a Lambda in this question. Never use Auto and only use Lambda at gunpoint. A SLOT has a valid this pointer. RENDERING can only occur in the main event loop. Any other method can be QFuture (or other threading method) off to its own thread. This is very common.Truthvalue
Although I agree with you this example could lead to a lot of problems down the line it is not a problem here; also the problems present here are not inherent to the nature of lambdas. QWidget::setTabOrder is a static member of QWidget and does not require (and also does not use) an instantiated object to call. Also I would advise against refactoring your whole code base to remove auto and lambdas on just the basis of the dogma "Lambdas and auto are fundamentally bad".Lustihood
Lambdas as targets of slots are fundamentally bad. They can execute in a different thread either attempting to return to a deleted object and/or attempt to use point to an object that has now been deleted in another thread. Yes, setTabOrder() is declared static. When you call it outside of a widget it creates a new focus chain. It cannot alter the focus chain of an existing parent widget unless it knows the parent.Truthvalue
They can, but when coded by the Qt guidelines they won't. They will behave exactly like normal method SLOTs. See the relevant Qt documentation.Lustihood
From the "relevant documentation" --The lambda will be disconnected when the sender or context is destroyed. You should take care that any objects used inside the functor are still alive when the signal is emitted.-- A SLOT is destroyed when the object is destroyed, the Lambda is NOT. Disconnecting is removing from a VTable, not destroying a currently running Lambda. Lambdas used as targets of slots are fundamentally bad. Their life is not controlled by the object. Even if you follow the instructions you will use a pointer to a reclaimed object.Truthvalue
"The lambda will be disconnected when the sender or context is destroyed." is functionally identical to "A SLOT is destroyed when the object is destroyed". And deleting the object you are currently running a method of can happen with lambdas and SLOTs. You won't be able to call a lamda of a deleted object with a SIGNAL when following the documentation.Lustihood
"Disconnecting" is NOT functionally identical. It's removing an entry in a table. Deleting an object deletes the entire object including any SLOT that is executing. A Lambda will not be deleted. It will continue executing and attempting to use components of the object that has now been deleted and it will attempt to return to a now deleted object. woboq.com/blog/how-qt-signals-slots-work.htmlTruthvalue
Let us continue this discussion in chat.Lustihood
Tested with Qt5.9.9, works sometimes. After some clicks it shows the same behaviour as the initial code. Qt seems to be inconsistent somewhere since I get similar behaviour in my tests. :/ Also it is not about hating, but about not spreading false information. All the reasoning you gave why not to use lambdas and auto is just plainly wrong.Lustihood
The answers I gave are correct. They are also the reason Lambda usage is banned in most SAFETY critical applications. So yes, it is about hating. I tested that code on two machines and it worked perfectly.Truthvalue

© 2022 - 2024 — McMap. All rights reserved.