A walk on the C++ Qt wildside

[Benjam901]

Hello all,

I was wondering if anyone could do me a favour and have a quick glance at my newest blog post about learning Model/View programming and the code itself.

I am looking for some feedback on potential issues that could arise from my programming, memory leaks, things that will go wrong in the future if not taken care of now etc.

https://www.benhearntechartist.com/single-post/2017/04/08/Lightweight-C-Dynamic-ModelView-QSortFilterProxyModel

Cheers,

Ben

[Justin Israel]

Hey Ben,

Thanks for sharing your post! I've gone over a little of it to try and give you the feedback you asked for. Let me know if you want me to expand on anything..

The first thing that stands out the most is that there is potential memory leak and design flaw in your CustomProxyModel.

You have a public vector of "filters", which gets allocated on the heap by your proxy, but your proxy class does not have a destructor and is not set up to manage that memory. So the class on its own leaks memory. But then if we look at the implementation of Multi_Array_Table class, we see that it contains logic for "stealing" the widgets from the vector of the proxy and taking ownership of them. On subsequent calls to that setup function it will delete the previous widgets and clear the vector. So this combination has a number of implications:

* The proxy model has a public setupFilters() method which will always create and push more widgets into a vector each time it is called.

* The proxy model has no idea when some or all of the memory will be deleted. If the Multi_Array_Table gets deleted and the proxy model is still around, being used by another model, it will have dangling pointers in its vector.

* The Multi_Array_Table class is relying on "stealing" ownership of widgets created by another class while that other class still maintains a public vector of the memory.

I don't feel a proxy model should be creating widgets. And it usually isn't appropriate in C++ to have public access to fields like that, since any caller can now have full access to modifying that vector. At the very least you should probably add destructors to your classes, and maybe make use of QPointer or QSharedPointer for tracking ownership in a more realiable way. It would probably be even a better step to not have your proxy rely on widgets, rather just pure data. If the view has the QLineEdits and can update the proxy with the new text data when they change, then your proxy can test QStrings instead of QLineEdits.

Other notes:

I don't often see the pattern of passing &Class::someSignal and &Class:someSlot when calling connect(). 

Is this something you have seen in other examples? The usual documented way involved SIGNAL("someSignal()") 

and SLOT("someSlot()").

In your CustomProxyModel::filterAcceptsRow, you have it iterating and building bitwise values. Would it not be simpler

to just return false when the string does not contain the regex, and then return true at the end?

In CustomProxyModel::setFilter, you are identifying the target filter by objectName, which seems a bit fragile. Wouldn't

just comparing the sender to the filterEdit work, since comparing the pointers would tell you that they are the same?

Boost seems like a huge hammer to introduce as a dependency here, just for using the recursive directory support in boost::filesystem. 

Could you use QDirIterator for the same purpose and ditch boost?

There sure are a lot of "util" files in the project. Some of which do really trivial things, such as just returning the string::c_str()

from a string. Others do some pretty inefficient work, like converting to one kind of vector, then iterating that vector to convert

to another kind of vector. They seem to obscure the top level calls and hide the costs. An example is convertToQStringList(), which 

really only needs to do something like:

    QStringList slist;
    slist.reserve(vec.size()); // optional
    foreach (const std::string &s, vec) {
        slist << QString::fromStdString(s);
    }

​

Justin

--
You received this message because you are subscribed to the Google Groups "Python Programming for Autodesk Maya" group.
To unsubscribe from this group and stop receiving emails from it, send an email to python_inside_m...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/f1f90e5d-18a2-44a5-94bc-66be89ea16c9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ben Hearn]

Hello Justin,

Thanks for the feedback it was exactly what I was after, some solid feedback on design flaws and mem leaks etc. is what I am in most need of at this stage of learning.

Should I always add destructors to any C++ class, QObject class or otherwise? I understand that the Qt ownership setup means that when the class is parented to A, if A is closed or destroyed then the class will be destroyed properly with it, is this a method that should not be trusted in its entirety and we should destruct each class just to be sure?

The proxy class I have implemented, the reason I used a public vector of structs is to keep track of which line edit and which QRegExp match together, the stack overflow example for multiple columns specifically connects a LineEdit and subsequent QRegExp together in the connect function and must also invalidate the correct filter. Is there another way to do this that does not involve this method?

In the setFilters function sending the QString from the LineEdit is what I am doing at the moment but I am iterating the Vector of structs to find the correct QRegExp. Also in the filterAcceptsRow my current level of understanding is that we must iterate over each column and get the strings from the LineEdits and compare them against the rows to validate if the strings exist in there. Is there a better way to do this also that I have not looked into?

Good call on the boost library, It is not even used, I will remove it and if I use it in any other programs for sure I will use QDirIterator.

The util functions you mentioned. I take it that it is not very good design to have such trivial calls such as convert to c_str()? Inefficient, not necessary etc?

The connect functions you mention, yes I saw it present in some QAction connect examples. I assumed this to be correct, I will change it and test.

- Ben

To unsubscribe from this group and stop receiving emails from it, send an email to python_inside_maya+unsub...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/f1f90e5d-18a2-44a5-94bc-66be89ea16c9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Python Programming for Autodesk Maya" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/python_inside_maya/IIdNOBuMonM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to python_inside_maya+unsub...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/CAPGFgA2dF6ftW46BOLdJuYeOqLJ4Q30Kk0YWmLkF%3D1zNwS_76w%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Tel - +46 76245 92 90 (Sweden)

LinkedIn: http://www.linkedin.com/pub/ben-hearn/50/a64/33b

[Justin Israel]

On Tue, Apr 11, 2017, 12:13 AM Ben Hearn <benandr...@gmail.com> wrote:

Hello Justin,

Should I always add destructors to any C++ class, QObject class or otherwise? I understand that the Qt ownership setup means that when the class is parented to A, if A is closed or destroyed then the class will be destroyed properly with it, is this a method that should not be trusted in its entirety and we should destruct each class just to be sure?

You should add destructor to classes that create dynamic memory on the heap (using new) that do not get cleaned up via the Qt parent child relationship. You are correct that the line edits get cleaned up by their parent. But you still have a number of cases where it's possible that it won't happen. My specific point was the way you are sharing the memory between the proxy and main window. Let's pretend you wanted to reuse this proxy in another setup. Now of you look at this proxy on its own, it leaks memory. It is only because of the main window stealing them and taking ownership that they get cleaned up. It's a bad idea to have one class hold a public vector to widgets which are owned and deleted by another class. They are tightly coupled in order to be correct. 

The proxy class I have implemented, the reason I used a public vector of structs is to keep track of which line edit and which QRegExp match together, the stack overflow example for multiple columns specifically connects a LineEdit and subsequent QRegExp together in the connect function and must also invalidate the correct filter. Is there another way to do this that does not involve this method?

Does it have to be a QLineEdit in the struct? If the main window owned the line edits, and your proxy just managed a QString value and regexp, could the main window wire up the text changed signals to a slot that updated the proxy with each text value? If you keep the proxy data-only it will be much safer and more reusable. 

In the setFilters function sending the QString from the LineEdit is what I am doing at the moment but I am iterating the Vector of structs to find the correct QRegExp. Also in the filterAcceptsRow my current level of understanding is that we must iterate over each column and get the strings from the LineEdits and compare them against the rows to validate if the strings exist in there. Is there a better way to do this also that I have not looked into?

Are you referring to my comment about building that bitwise bool? If so, I was just saying the logic could be simpler. You are building a bool and if any of the patterns don't match you already know the answer and can bail. If you make it through all the filters matching then you have the other answer. 

Good call on the boost library, It is not even used, I will remove it and if I use it in any other programs for sure I will use QDirIterator.

The util functions you mentioned. I take it that it is not very good design to have such trivial calls such as convert to c_str()? Inefficient, not necessary etc?

If it's super trivial, then it's a waste of an extra function call that just hides simple details. It would make sense to create functions if you wanted to abstract the implementation details so you only have to change that logic in one place if needed. If your abstraction serves no real benefit then it is just needless indirection that someone has to read to follow the logic. In the case of just returning a c_str() for a string, the char* isn't even valid beyond the life of the string right? So it doesn't make much sense to hide that detail. 

To unsubscribe from this group and stop receiving emails from it, send an email to python_inside_m...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/f1f90e5d-18a2-44a5-94bc-66be89ea16c9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to a topic in the Google Groups "Python Programming for Autodesk Maya" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/python_inside_maya/IIdNOBuMonM/unsubscribe.

To unsubscribe from this group and all its topics, send an email to python_inside_m...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/CAPGFgA2dF6ftW46BOLdJuYeOqLJ4Q30Kk0YWmLkF%3D1zNwS_76w%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Tel - +46 76245 92 90 (Sweden)

LinkedIn: http://www.linkedin.com/pub/ben-hearn/50/a64/33b

--

You received this message because you are subscribed to the Google Groups "Python Programming for Autodesk Maya" group.

To unsubscribe from this group and stop receiving emails from it, send an email to python_inside_m...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/CAM2ybkWqzeLYh7ub7g3xOH-uG5AuaLbpZDqaHVzZKYJ1dsBZAw%40mail.gmail.com.