need ideas for list/table implementation

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

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.