need ideas for list/table implementation

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

Mother of God!

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:

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).
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...