fix (GT-96): parameters tab search past first

[gordon-epc]

On the Parameters tab, allow search to go past the first found item.

[altendky]

Other than a couple attrs options and the meandering brainstorming about attrs-with-Qt... what was the need for using inheritance rather than the layering of the proxies as before? Is that what fixed the issue, or something else I missed.

[altendky]

(tl;dr probably do nothing, but I felt like thinking about this so I wrote this up anyways)

So, I just learned that you can pass properties to the constructors with PyQt (python-qt-tools/PyQt5-stubs#147 (comment)). I don't know that I want to use that, or if it works with PySide (maybe relevant someday here, definitely relevant elsewhere for us), but figured I'd share anyways. Point is that this could get moved up to the attribute definition then.

wildcard: QtCore.QRegexp = attr.Factory(lambda: QtCore.QRegexp(patternSyntax=QtCore.QRegexp.Wildcard)

But that still doesn't get rid of the .__attrs_post_init__() so... whatever... until we write a qt-specific attrs decorator... someday. Or maybe your change below leads somewhere good. Taking it a step further, perhaps make a basic base class that handles the .__attrs_post_init__() then leave all the other classes clean (at least those that only need parent passed up).

@attr.s(auto_attrib=True)
class AttrsQObject:
    _parent: QtWidgets.QWidget = None

    def __attrs_post_init__(self):
        super().__init__(parent=self._parent)

Note the 'private' ._parent to avoid shadowing the QObject.parent() method. Yeah, that issue pre-exists this change, just noticed it. Ah, inheritance...

[gordon-epc]

I held off changing the wildcard declaration + initialization to a one liner. I did do the _parent. Again that's a good find.

What I found is that no matter what, there is no way to avoid adding the __attrs_post_init__ method. The inherited class init method has to be called and as far as I can tell the only way to do that is making a call through __attrs_post_init__.

Really, it doesn't make much sense to have the base class. It's not saving any lines of code, that's for sure. Let me know what you think.

[altendky]

Having a general AttrsQObject, or whatever, is a bigger picture thing than this PR or these two classes. The sort of thing that is of at least uncertain value to backport, but ... might (?) be a good thing for future development. It would clearly avoid the .parent shadow issue in one place and allow perhaps many other classes to not have a .__attrs_post_init__() which would encourage them to do less in there which is one of the things I've accepted as good about attrs. No more, 'well I already have one so... do a bit more!'. :]

But yeah, big picture, various opinions, not really to be dealt with in this PR.

[gordon-epc]

Also, to answer your question on the need for inheritance. The real fix for this was to ditch inheriting from QIdentityProxyModel, which prevented the search from working properly. The class did have functionality for highlighting differences in the table. So it made sense (in my mind) to inherit from the working search code (now called SortFilterProxyModel), which in turn inherits QSortFilterProxyModel.

Now, in my research for this, I did discover this paradigm of foregoing inheritance in Python (and elsewhere) for interfaces. This goes against everything I've been taught. I'm going to explore this paradigm more.

[altendky]

I see composition over inheritance as a bit separate from layering the proxies or not. Qt is _all_ about inheritance yet they refer to setting up proxies in pipelines.

https://doc.qt.io/qt-5/model-view-programming.html#using-proxy-models

They can also be used to process the information obtained from other proxy models in a pipeline arrangement.

But sure, that is... not really a 'thou shalt stack proxies and not multiply inherit them'. Just a mild suggestion of one option. :]

Anyways, yeah, a few years ago I accepted the habit of at least tending away from inheritance. So far I haven't regretted it. There are other issues you hit that way, no doubt. And I don't straight up 'ban' inheritance (in so much as I have any such authority anyways). One factor I see is that historically C++ was kind of the big name in OOP and to have a collection of This and That you would have to have had the collection be set to contain some common inheritance ancestor of the two. With Python, it is sufficient that all the objects in the list simply have the expected methods and attributes, if any, that code operating on the collections exercise. tl;dr, inheritance used to be roughly mandatory, now it is more optional. Let's try not-it out.

In this particular case, there really isn't much code to share, I don't think? When the inheritance was switched from QIdentityProxyModel to SortFilterProxyModel only the .__attrs_post_init__() was removed and that is mostly independent of either of these.

---

Down to the core issue that inspired this PR... past the vast void between the inheritors and the composers... back to an actual problem... :] Do you know what bit of QIdentityProxyModel was causing trouble while our two proxies were still separate?

I just tried here in Linux and noticed the output below. Had you seen this? Maybe it is more about some code I have that is mishandling the two layers of proxies? Switching to inheritance had the effect of reducing down to a single proxy layer. And, I do have messy code that isn't really respecting the intended use of the proxies. :[ Sorry... again. I guess one test would be to just swap inheritance from QIdentityProxyModel to QSortFilterProxyModel and see if it fails the same way as before.

qt_message_handler: f:None l:0 f():None
  WARNING: QSortFilterProxyModel: index from wrong model passed to mapToSource

Now that I've suggested more stuff to look into... I'll volunteer to do some of the digging and checking since I'm the one resisting the change here. It just doesn't feel like there _should_ be an issue with QIdentityProxyModel. If nothing else, my compulsion is to know a bit more detail as to why.

If not that, then I dunno, maybe this is somehow related? QIdentityProxyModel::match inappropriately mixes use of proxy model index and source model data method

Side note, maybe we should update PyQt (separately). Looks to be 5.13.0 in stlib and 5.14.1 in st.

[altendky]

Having a general AttrsQObject, or whatever, is a bigger picture thing than this PR or these two classes. The sort of thing that is of at least uncertain value to backport, but ... might (?) be a good thing for future development. It would clearly avoid the .parent shadow issue in one place and allow perhaps many other classes to not have a .__attrs_post_init__() which would encourage them to do less in there which is one of the things I've accepted as good about attrs. No more, 'well I already have one so... do a bit more!'. :]

But yeah, big picture, various opinions, not really to be dealt with in this PR.

[gordon-epc]

I did notice the warning message that was printing and I no longer get that message during search with this code.

The reason I ditched QIdentityProxyModel is because the docs seem to suggest that the proxy isn't used for sort/filter and presumably search? Looking at the code, the use of QIdentityProxyModel seemed to be the difference between search working on the TxRx tab and not working on the Parameters tab. I could be wrong!

I also made the assumption that the SortFilterProxyModel class contained code to make search work. I didn't look carefully at the code, but it must be doing something? Since the Parameters tabs uses both search and cell highlighting, I figured inheriting SortFilterProxyModel with the existing highlight code made sense. I could be wrong!

Last point, I'm not sure what QIdentityProxyModel was doing to assist the Parameters table to highlight cells. From what I can tell, the Parameters table is working properly without it (or rather, after being replaced by SortFilterProxyModel(QSortFilterProxyModel)).

[altendky]

QIdentityProxyModel made the diff proxy be a proper, albeit passthrough, proxy upon which it could just add modification of the coloring. Anyways, I'll look in the morning and see if I can satisfy my curiosity.

[gordon-epc]

Per suggestions, I added printing of the stack trace in the message handler and also adjusted the code in the search_view() method.

[altendky]

The changes to search_view() were sufficient to fix the issue since they corrected the layered proxy/model traversal. Do we need to couple the two proxies together by inheritance still? A call is welcome if that would make a better discussion. Otherwise I think I'm good.

[altendky]

I had imagined this being a generic QObject inheriting thing but I kind of forgot about how we want to inherit from something else... I guess maybe it would still work and then the other class would just get mixed in?

@attr.s(auto_attribs=True)
class AttrsQObject(QtCore.QObject):
    _parent: QtCore.QObject = None

    def __attrs_post_init__(self):
        super().__init__(self._parent)

@attr.s(auto_attribs=True)
class SortFilterProxyModel(AttrsQObject, QtCore.QSortFilterProxyModel):

This is really a big picture question though and I can't say I'm hot on the multiple inheritance I just typed here... maybe it isn't worth it and a pattern is the right approach.

[altendky]

Thanks for the patience... :|