need ideas for list/table implementation

[mzimmers]

I need to add a list or table (I'm not sure which) to the UI of a program I'm developing. This is to display a (fairly short) list of connected devices. The information on each entry would be a few columns, such as name, type, most recent activity. The table would be updated a few times per second. Some color coding (based on most recent activity) and sorting would be nice additions.

I did a little reading on Qt's model/view programming, and it seems rather elaborate for my needs, though maybe I have to go that route. Before I jump in, I'd be interested in hearing any other ideas on how best to implement this.

Thanks...

[JonB]

@mzimmers
So http://doc.qt.io/qt-5/qtablewidget.html#details or a http://doc.qt.io/qt-5/qtableview.html would indeed be my first suggestion.

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

I did a little reading on Qt's model/view programming, and it seems rather elaborate for my needs, though maybe I have to go that route.

That would be my go-to solution. You would need to implement only a couple of methods from QAbstractTableModel and use the standard view that goes with it.

[mzimmers]

OK, if I'm going to attempt this, I'll need a good example to follow. Is the frozencolumn example (found in Creator->Welcome->Examples) a good one to use as a model (so to speak)?

Thanks...

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

OK, if I'm going to attempt this, I'll need a good example to follow. Is the frozencolumn example (found in Creator->Welcome->Examples) a good one to use as a model (so to speak)?

Yes it should do, although it seems a bit elaborate for what you describe. If you need to have simply a list also consider QAbstractListModel (e.g. puzzle example) which provides even more methods already overriden, so there's less to write. The model subclassing page should also be of use to check.

[mzimmers]

I'll need a table (2 dimensions of data). I'm working through an example I found in an old 4.8 tutorial...it's starting to make sense. I'm curious, though, about how this works with the traditional Qt worker/widget paradigm. Currently, my worker sends table data to the widget (via the signal mechanism). Obviously, my widget needs to update the table, but it can't do this directly because the data structure is contained in my table class. Do I manually create methods for the widget object to call, or is there something automatic that does this for me?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

I'm curious, though, about how this works with the traditional Qt worker/widget paradigm.

You lost me here. Could you rephrase? I don't get what you're asking.

[mzimmers]

So, an application might typically have a worker object and a widget object. Now, we're introducing the table object (based on QAbstractTableModel). I'm accustomed to the worker receiving data from a remote device, and passing that along to the widget. Now, however, the widget doesn't directly process that data, but somehow needs to get it into the table. How is this best accomplished?

[SGaist]

Hi,

In this case, your worker should work with your table model. When implemented properly, the model signals that it has new/modified data through begin/endInsertRows, dataChanged and their signal friends. All views that you set this model on will then update themselves.

[mzimmers]

Hi SGaist -

Just to be clear, my table is read-only to the user. The only changes to the table will be initiated by the worker object (via a socket message from the target device). Does the worker directly signal the model object?

[SGaist]

The fact that it's read-only is a detail. You can disable all edit triggers and even make the setData method of your model a "no-op" method.

So, yes, your worker can still work with your model directly.

[mzimmers]

So, the examples I'm looking at are somewhat lightweight on the data. It isn't clear to me if/how the worker is supposed to store a copy of this data. I don't see where the call to insertRows() provides any actual data.

Each row in my table will consist of 3 columns, all strings. When I want to update a row, what do I call to do this? The first column will be like my primary key.

[SGaist]

Are you using your model as wrapper on top of a custom data structure or as holder of said data ?

Your worker can feed the model, it doesn't need to store anything.

[mzimmers]

I don't know how to answer that; I'm new to this model/view stuff. But the intention is simply to pass information along from the worker to the UI. If i can bypass the need to store the information locally, so much the better. But I don't see how to do that.

I gather that I'm supposed to re-implement the insert rows function, but I don't see how to use that to actually inject data into the model.

Also, now that I've bypassed the widget, my table is showing up in a separate window. Do I correct this by passing a different parent object (like the Ui) to its constructor?

EDIT:

Oh, I think I get it a little better now. So, the class derived from the table model holds the data. The table object receives the data updates from the worker and stores them, then emits the dataChanged() signal. And the data() function then conveys the current data to the UI. That about right?

So, am I responsible for maintaining a row index for my records? (Since data uses an int to identify a row.) I could store my table data in a hash, with an int as the key, and a structure containing the strings as the value.

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Oh, I think I get it a little better now. So, the class derived from the table model holds the data. The table object receives the data updates from the worker and stores them, then emits the dataChanged() signal. And the data() function then conveys the current data to the UI. That about right?

Yep, quite correct.

So, am I responsible for maintaining a row index for my records? (Since data uses an int to identify a row.)

Yep.

I could store my table data in a hash, with an int as the key, and a structure containing the strings as the value.

You could, but if your keys are sequential and uninterrupted a hash isn't exactly efficient, maybe a vector.

[mzimmers]

OK, I seem to have the basics working. Thanks for the help. Now..what do I have to do to get my table to not appear as a separate window?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Now..what do I have to do to get my table to not appear as a separate window?

Give it a parent widget and/or add it to an active layout (which will give it a parent widget).

[mzimmers]

The table belongs to the worker class, which doesn't currently have knowledge of the main widget class. Is there a clean way to do this?

[mzimmers]

My program isn't working quite correctly, and I'm wondering if it has to do with my non-use of insertRows().

Do I correctly understand that in order to insert a new row into my table, I need to do something like this:

// part of the subclass update function

devices.insert({row, device}); // my local copy of the data
beginInsertRow(parent, rowCount(), rowCount());
insertRow(rowCount(), 1, parent);
endInsertRow();

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

The table belongs to the worker class

If you mean the table view belongs the the worker object (which is presumably living in another thread), then this is wrong. You must keep the GUI classes in the GUI thread. If you mean the table model is in the worker thread then I think it is okay. What I meant is that you should parent the table view to a widget (and add it to a layout), so it's not a native widget and doesn't get its own window.

My program isn't working quite correctly, and I'm wondering if it has to do with my non-use of insertRows().

Perhaps, can you share the actual code?

Do I correctly understand that in order to insert a new row into my table, I need to do something like this:

insertRow calls the virtual insertRows, so it's a convenience method. You should implement insertRows for your model in which you'd call beginInsertRows before saving the data, and endInsertRows after that.

[mzimmers]

OK, I'm doing it wrong.

My table view is in my worker. If I move it to my widget, how do I give it the model information in the setModel() call -- do I do something like pass the model as an argument to the widget constructor?

I'll post some code in a bit, when it's more "post-worthy." In the meantime, I'm still a little unclear on the insertRows() function I need to write. Does this operate on my local copy of the data? So, the pseudocode would be something like:

void Devices::MyInsertRow()
{
    devices.insert(etc)
}
...
beginInsertRow(parent, rowCount(), rowCount());
myInsertRow();
endInsertRow();

[VRonin]

@kshegunov said in need ideas for list/table implementation:

If you mean the table model is in the worker thread then I think it is okay.

It's not. The view calls methods of the model (data() being the most obvious one) directly and that's a race condition (and no, you can't just serialise access to your internal data as QPersistentModelIndex still causes a race condition as soon as you try to sort)

@mzimmers I think a lot of our confusion comes from you using the ambiguous term "table object" that doesn't tell us if you are talking about a model or a view

[mzimmers]

Yeah, sorry about that. I have created this class:

typedef std::unordered_map<int, DeviceDetails> ModelData;

class Devices : public QAbstractTableModel
{
private:
    ModelData devices;
...

And I instantiate an object from this class in my Worker object/thread.

I also have the table view in my Worker class:

class Worker : public QThread
{
    Q_OBJECT

private:
    QTableView tableView;
    Devices devices;

Kshegunov said that the table view needs to be in the widget instead, which makes total sense, but then there's the matter of how to call setModel() (specifically what to pass to it.)

[SGaist]

Depending on what your Worker class does, it would be better to have it as a member of Devices rather than the way it is now.

There's no need for Worker to know anything about the GUI elements that will be using Devices as model.

[kshegunov]

Wait, hold your horses for a second, there's something fishy here. I thought worker is a worker object ... but it's not it's a thread, that makes very little sense ... did you override run() of that class? If so what do you do there? If not how is this a separate thread? (NOTE you shouldn't call moveToThread on a QThread, that's completely bogus).

[mzimmers]

Yes, I did:

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    Widget widget;
    Worker worker;
    int rc;

 // a bunch of connects here
    widget.show();
    worker.start();
    rc = a.exec();
    worker.wait();
    return rc;
}
...
void Worker::run()
{
    running.ref(); // set to value of 1
    while (running)
    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(nullptr, buffIn);
            emit(newMessage(&msg));
// some other stuff here
        }
        Sleep(10);
    }
    emit reachedEndOfThread();
}

Is this not correct?

[VRonin]

Is this not correct?

No. It isn't.

• You have race conditions with 99.99% probability (your 0.01% hope is that running, len, sm, buffIn are all thread safe and Message is at least reentrant).
• You are emitting the address of a temporary variable (msg) from a secondary thread
• tableView and devices live in the thread that calls Worker's constructor (that's the reason why you are not getting segfault as soon as you try to call tableView.show()).

Having non-top-level widget allocated on the stack is always a headache as you have to make sure the parent-child does not try to delete the child before it goes out of scope.

First thing first: let's nail down how to use QThread https://mayaposch.wordpress.com/2011/11/01/how-to-really-truly-use-qthreads-the-full-explanation/

[mzimmers]

*This should immediately show why the recommended way of using QThreads in the documentation, namely to sub-class it and implement your own run() function, is very wrong. *

Really and truly?

So, the docs are wrong? Still?

http://doc.qt.io/qt-5/qthread.html#details

Another way to make code run in a separate thread, is to subclass QThread and reimplement run(). For example:

I wish I knew who to believe...even better, there wouldn't be uncontested disagreement on this subject.

But I can go ahead and re-implement it as per the blog.

[VRonin]

The docs are not wrong, I think they are just misleading.
Subclassing run() is an option but that doesn't imply any other method/member of the QThread subclass or the instances on this subclass live on a separate thread as you, me and a lot of other people intuitively thought the first time we approached QThread

[mzimmers]

OK, here's my changed main():

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    Widget widget;
    QThread* thread = new QThread;
    Worker* worker = new Worker();
    worker->moveToThread(thread);
 ...
    QObject::connect(thread, &QThread::started, worker, &Worker::process);
    QObject::connect(&widget, &Widget::quitButtonPushed, worker, &Worker::doQuit);
    QObject::connect(worker, &Worker::finished, thread, &QThread::quit);
    QObject::connect(worker, &Worker::finished, worker, &Worker::deleteLater);
    QObject::connect(thread, &QThread::finished, thread, &QThread::deleteLater);

    widget.show();
    thread->start();
    rc = a.exec();
    return rc;
}

First order of business: how does this look?
Second order of business: why does my quit button no longer work?

void Widget::on_quitButton_clicked()
{
    emit quitButtonPushed(0);
}
...
void Worker::doQuit()
{
    emit finished();
}

Thanks. Once I get this thread stuff straightened out, I think I'll close this topic and start a new one on the subject of model/view.

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

This should immediately show why the recommended way of using QThreads in the documentation, namely to sub-class it and implement your own run() function, is very wrong.

Really and truly?
So, the docs are wrong? Still?
I wish I knew who to believe...even better, there wouldn't be uncontested disagreement on this subject.

Of course it isn't wrong to subclass and reimplement run(), she's overplaying it. However, both approaches have their specifics that should be taken into account, which @VRonin hinted. Firstly and most importantly is that QThread is not a thread, it's a class that manages a running thread. This also means that the object that manages the thread is not in the running thread (in the sense of thread affinity), and the distinction is important.

This implies for each of the approaches:

1. When you're subclassing and overriding run() that is your thread, literally the QThread::run() override is the whole thread. You can't store things in the thread object, because the thread object is "living" in another thread and all its slots will be called from a thread different than the one that is QThread::run(). Additionally, since there's no event loop in that case you can't have any events posted in the thread (slots can't be triggered from signal emissions coming from other threads with the exception of direct connections, signal emissions are fine to do; no timers can be run in that thread and so on).

   This approach is usually employed for the rarer case when you need to run an imperative code that doesn't care much about events and such, and just does something that it communicates to the outside world. E.g. some number crunching thread that does its business and spits out results. The thread ends with you returning from QThread::run(); also meaning you should handle the termination condition (semi-)manually (e.g. QThread::requestInterruption and the corresponding getter).

2. When you're using a worker object, you have a running event loop that can process queued events and have timers running. However, you should make sure that you're not blocking the event loop for too long ... yeah, every time I see Sleep a little piece of me dies in horrible pain ...

   This approach is the usual choice, and allows you nice separation of what data is accessed in what thread - i.e. the data contained in the instance of the QObject of some thread is touched only from that same thread. Connecting with signal-slots between the threads allows for Qt to give you data access serialization out of the box, without you needing to use mutexes, semaphores and other such nastiness. In contrast, though, concurrency is hurt a bit compared to 1, but that's irrelevant for 99.9% of possible applications.

First order of business: how does this look?

At a glance looks okay.

Second order of business: why does my quit button no longer work?

Do you run an endless loop in Worker::process? If so, you're blocking the event loop. It ain't gonna work like that.

[mzimmers]

Thanks for the detailed explanation, kshugenov.

So, regarding cross-thread communication, if the worker thread's infinite loop is blocking the event loop, does that imply that the worker won't respond to any signals at all? And if so, what's the recommended method for killing another thread? I tried this:

    QObject::connect(&widget, &Widget::quitButtonPushed, thread, &QThread::quit);

And even that doesn't work.

Thanks...

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

So, regarding cross-thread communication, if the worker thread's infinite loop is blocking the event loop, does that imply that the worker won't respond to any signals at all?

Yes, exactly. Blocking the event loop means no event processing, means no queued signal-slot calls, means slots won't be called until the event processing resumes. Any signals that are connected to slots in the worker through Qt::AutoConnection, Qt::QueuedConnection, or Qt::BlockingQueuedConnection are not called. This doesn't apply to Qt::DirectConnection, because direct connections don't require an event loop and event processing - the same as a direct calls.

And if so, what's the recommended method for killing another thread?

This is the correct way of stopping the thread when you're using the worker object, you just have to rework a bit your code so it doesn't block the event loop. For example substituting the while (true) for a timer with some timeout (or zero timeout). :)

[mzimmers]

I do have a sleep in my loop:

void Worker::process()
{
    int len;
    while (true)
    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(nullptr, buffIn);
            emit(newMessage(&msg));

            // if message is a heartbeat, send an ack.
            if (msg.getType() == MSG_HEARTBEAT)
            {
                sendHeartbeatAck();
                updateDeviceTable(msg);
            }
        }
        else
        {
            SocketState ss = sm.getSocketState();
            if (ss == SOCKET_DISCONNECTED)
            {
                //sm.init();
            }
        }
        Sleep(10);
    }
    //emit reachedEndOfThread();
}

(I renamed my worker routine from run() to process() to agree with the example I'm trying to follow.)

But this doesn't work. Are you saying it needs an actual timer (sleep() isn't any good)?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

But this doesn't work. Are you saying it needs an actual timer (sleep() isn't any good)?

sleep() makes me cringe every time. Yes, I am saying that sleep() puts the thread to sleep and it does nothing, no event processing, literally nothing. You can't even interrupt that! In your case, looking at the code, the very simple solution would be to have something like this:

class Worker : public QObject
{
public:
    Worker();

public slots:
    void start();
    void process();

private:
    QTimer pollTimer;
};

Worker::Worker()
    : pollTimer(this) //< Important so the timer is moved to the new thread along with the worker
{
    pollTimer.setInterval(10000); // 10 seconds or so
}

void Worker::start()
{
    pollTimer.start();
}

void Worker::process()
{
    int len = sm.recv(buffIn, sizeof(buffIn));
    // And more of whatever it is you do INSIDE the `while (true)`
}

Also you seem to be using the native sockets, why?

PS.
Connect the QThread::started to Worker::start and it should be working out of the box.

[mzimmers]

Thanks for the example. Don't I need to connect the timer to my connect() routine somewhere, though?

[kshegunov]

@mzimmers said in need ideas for list/table implementation:

Don't I need to connect the timer to my connect() routine somewhere, though?

Yeah, I missed it, sorry about that. You should have one such line:

QObject::connect(&pollTimer, &QTimer::timeout, this, &Worker::process);

in Worker's constructor.

[mzimmers]

OK, I think I'm almost there. Now, what's the recommended behavior of the slot that receives the quit button signal? exit()? quit()? Something else?

[kshegunov]

The quit button signal you should connect to QThread::quit, just as you had shown above. That's the way to do it.

[mzimmers]

Hmm...it's not working. I read this in the QThread page:

This function [quit] does nothing if the thread does not have an event loop.

Do I need to explicitly declare/start an event loop?