collections, inheritance and composition

[andrea-bistacchi]

After what we discussed in the call on June 25th, 2024, I realized that maybe we can avoid multiple inheritance in the base superclass used to derive collections by using composition. This can be done if the QAbstractTableModel becomes a property of the BaseCollection, instead of being its parent class.

This is already done for another fundamental component of collections: the DataFrame.

from pandas import DataFrame

[...]

    @property
    def df(self) -> DataFrame:
        """
        Get or set the dataframe of the Collection
        """

        return self._df

    @df.setter
    def df(self, df: DataFrame):
        self._df = df

This should not force us to rewrite a lot of code. I guess that if QAbstractTableModel becomes a property like collection.qatb, what we need is just to add .qatb where needed. Correct?

The discussion is open!

[gbene]

Mhhhh... The problem is that we use the GeologicalCollection as model for the GeologyTableView in the project window.

        """Create the geol_coll GeologicalCollection (a Qt QAbstractTableModel with a Pandas dataframe as attribute)
        and connect the model to GeologyTableView (a Qt QTableView created with QTDesigner and provided by
        Ui_ProjectWindow). Setting the model also updates the view."""
        self.geol_coll = GeologicalCollection(parent=self)
        self.proxy_geol_coll = QSortFilterProxyModel(self)
        self.proxy_geol_coll.setSourceModel(self.geol_coll)
        self.GeologyTableView.setModel(self.proxy_geol_coll)

And I think that in setSourceModel you need to pass a compatible class (like QAbstractTableModel) with the QT methods (data, headerData etc...) defined.

What we could try is to create a BaseTableModel class (inheriting from QAbstractTableModel) that has as the only parameter a parent and has all the QT methods modified with self.parent... We then set this BaseTableModel class as the qatb property and passing self as the parent of the BaseTableModel (I don't know if you were thinking this or something else!).

This could be useful also if we need the BaseTableModel in other parts not related to the Collections.

[andrea-bistacchi]

Or maybe also this proxy stuff, that is required to have sorting on the table view columns, could go in the collection?

[gbene]

Yes we could set the property qtab to directly return the proxy_coll and set it in self.GeologyTableView.setModel. I will try it now!

[gbene]

Hi so I finally figured out how to make it work and now it should be working with this last push d9fb189 (note: I am working from a new class_refactoring branch derived from windows_refactoring). Now we can access the tablemodel or the proxytablemodel with the properties table_model and proxy_table_model.

The table model is a BaseTableModel class that inherits from QAbstractTableModel and it is initiated automatically when the collection is created. It accepts as the first input the parent of the class (i.e. ProjectWindow) and as a secondary input the collection itself.

While doing this I have also modified the selected_uids property in the project_window and simplified a bit the code. Before we were doing:

        if self.shown_table == "tabGeology":
            selected_idxs_proxy = self.GeologyTableView.selectionModel().selectedRows()
            for idx_proxy in selected_idxs_proxy:
                selected_idxs.append(self.proxy_geol_coll.mapToSource(idx_proxy))
            for idx in selected_idxs:
                selected_uids.append(
                    self.geol_coll.data(index=idx, role=Qt.DisplayRole)

however this is a bit redundant and can be compacted to:

        if self.shown_table == "tabGeology":
            selected_idxs_proxy = self.GeologyTableView.selectionModel().selectedRows(column=0)  # this will always give rows that have selected the column 0 (in this case uid). By changing the column=0 to another index it will give the value in another column
            for idx_proxy in selected_idxs_proxy:
                selected_uids.append(idx_proxy.data())

Moreover, I do not why, but by doing this refactoring, mapToSource was not working properly and so I was forced to change the code (however I think this is better since you only need a loop instead of two!)

[andrea-bistacchi]

Seems solved and verified after push 864eecf