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 On 9 April 2017 at 01:26, Justin Israel <[email protected]> wrote: > 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 <http://doc.qt.io/qt-4.8/qdiriterator.html> 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 > > > On Sun, Apr 9, 2017 at 7:18 AM Benjam901 <[email protected]> wrote: > > 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 > > -- > 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 [email protected]. > To view this discussion on the web visit https://groups.google.com/d/ > msgid/python_inside_maya/f1f90e5d-18a2-44a5-94bc- > 66be89ea16c9%40googlegroups.com > <https://groups.google.com/d/msgid/python_inside_maya/f1f90e5d-18a2-44a5-94bc-66be89ea16c9%40googlegroups.com?utm_medium=email&utm_source=footer> > . > 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 > [email protected]. > To view this discussion on the web visit https://groups.google.com/d/ > msgid/python_inside_maya/CAPGFgA2dF6ftW46BOLdJuYeOqLJ4Q > 30Kk0YWmLkF%3D1zNwS_76w%40mail.gmail.com > <https://groups.google.com/d/msgid/python_inside_maya/CAPGFgA2dF6ftW46BOLdJuYeOqLJ4Q30Kk0YWmLkF%3D1zNwS_76w%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > > 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/python_inside_maya/CAM2ybkWqzeLYh7ub7g3xOH-uG5AuaLbpZDqaHVzZKYJ1dsBZAw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
