QTableView with custom model: color of disabled items

mpergand

@JonB you're welcome.

MVs are intended to allow multiple views onto the model,

I have no experience with that and it's a little bit nebulous, for me model=data, several views can share the same data and show the data in different ways with specific sorting, filtering etc.
I am confused with the Qt terminology.

I must confess, I never understood/liked the MVD paradigm from the beginning.

JonB

@mpergand said in QTableView with custom model: color of disabled items:

several views can share the same data and show the data in different ways

That's the point. You made your model's constructor take a parameter of a widget parent (like a QTableView). Then how can that model be used in multiple widget views? It cannot. So your approach breaks the ability for this model to be used in any view other than that (single one) passed when it was constructed.

For the record: for MVC ,in Qt's MV-only paradigm you can write "Controller" logic either in its own class which has access to both model & view instances or in the view class which is allowed to access the model. What you are not supposed to do is write it in the model class.

artwaw

@mpergand Rule of thumb: model provides the data, nothing else. Delegate is responsible for rendering the data. I agree it is often tempting to make a shortcut, like one you proposed, but this will bite back in the least expected moment. Better off to stick with the paradigm even if it doesn't seem to make sense in a given moment and involves a bit more work.

StarterKit

Sorry for delay, it is weekend time... I'll try to answer one by one.

@SGaist said in QTableView with custom model: color of disabled items:

Hi,
You are talking about both QTableView and QTableWidget, which one is it ?
Which version of Qt are you using ?
On which platform ?
Can you provide a minimal compilable example that shows that behaviour. From the looks of it, you might be using Python, in that case a minimal script.

Ok, I'm on latest stable 6.2.2 now with PySide6 for python. I have it on Linux and Windows.
Below is the simple python example.
Without setDisabled() it shows pretty normal window:
enabled
But after setDisabled(True) I have:

As you may see - green and red numbers are not greyed out.

from PySide6.QtCore import Qt, QAbstractTableModel
from PySide6.QtWidgets import QWidget, QTableView, QVBoxLayout, QApplication
from PySide6.QtGui import QBrush

class MyWindow(QWidget):
    def __init__(self, *args):
        QWidget.__init__(self, *args)
        self.setGeometry(300, 200, 300, 200)
        self.table_model = MyTableModel(self)
        self.table_view = QTableView()
        self.table_view.setModel(self.table_model)
        self.table_view.resizeColumnsToContents()
        self.layout = QVBoxLayout(self)
        self.layout.addWidget(self.table_view)
        self.setLayout(self.layout)
        self.table_view.setDisabled(True)


class MyTableModel(QAbstractTableModel):
    header = ['Watch', 'Price, $']
    data_list = [
        ('Amazfit Pace', 100.0),
        ('Garmin Forerunner 245', 250.0),
        ('Garmin Fenix 6', 500.0)
    ]

    def __init__(self, parent, *args):
        QAbstractTableModel.__init__(self, parent, *args)
    def rowCount(self, parent):
        return len(self.data_list)
    def columnCount(self, parent):
        return len(self.data_list[0])
    def data(self, index, role):
        if not index.isValid():
            return None
        if role == Qt.DisplayRole:
            return self.data_list[index.row()][index.column()]
        if role == Qt.ForegroundRole and index.column() == 1:
            if self.data_list[index.row()][1] > 300:
                return QBrush(Qt.red)
            if self.data_list[index.row()][1] < 200:
                return QBrush(Qt.green)
        return None
    def headerData(self, col, orientation, role):
        if orientation == Qt.Horizontal and role == Qt.DisplayRole:
            return self.header[col]
        return None

app = QApplication([])
win = MyWindow()
win.show()
app.exec()

StarterKit

@artwaw said in QTableView with custom model: color of disabled items:

@mpergand Rule of thumb: model provides the data, nothing else. Delegate is responsible for rendering the data. I agree it is often tempting to make a shortcut, like one you proposed, but this will bite back in the least expected moment. Better off to stick with the paradigm even if it doesn't seem to make sense in a given moment and involves a bit more work.

I can't fully agree with it. First of all - I'm legally able to return color from data() method() as Qt.ForegroundRole is definded.
Second - I may say that color of my item is also a piece of data (and it really is - i.e. I may provide it as a separate field for delegate but ForegroundRole gives a smoother way to do it)

StarterKit

@JonB said in QTableView with custom model: color of disabled items:

I tested this at least on a QTableWidget I have, and agree I saw items with a color supplied from data() not showing as "dimmed/greyed". It seems that only applies if no color is specified, whether that is right or wrong.
Given which, the only correct way to go is indeed to provide a QStyledItemDelegate as a custom delegate, because only a delegate has access to the state of the parent view. A minimal delegate to do the desired here is actually only a small number of lines of code.

Thanks for your efforts. I assume you got similar result to what I posted above.
In reality I have more complex coloring rules applied and some months ago I handled it with QStyledItemDelegate descendants. I had a lot of different delegates here and there, it was quite a mess.
Then I got a better grasp on Qt MV paradigm and re-wrote my models. I discovered ForegroundRole/BackgroundRole options and got rid of almost all of my delegates (I still have several but for specific things).
This is why I have no desire to go back to that messy code that did only minor things...
I fully understand that it isn't a bug. But from my point of view this behavior of QTableView isn't proper. Is it possible to report it as enhancement proposal somewhere?
(I probably may do a code change by myself... the only problem I don't have C/C++ compiler at hand for last couple of years...)

StarterKit

I just checked Qt.BackroundRole to be sure - I can confirm it has the same behavior. I.e. cell background is not grey:

I think QTableView and probably other widgets should be improved.
One simple approach I may propose - to ignore colors provided by ForegroundRole/BackgroundRole if widget is disabled.
As more complex thing - color might be "dimmed".
Without such improvement, text may even occasionally become invisible after disabling of widget (if custom foreground will match with disabled background or vice versa)

Christian Ehrlicher

@StarterKit said in QTableView with custom model: color of disabled items:

I think QTableView and probably other widgets should be improved.

Feel free to provide a patch for it and find people who think it's a bug.

Since a model doesn't know anything about enabled/disabled you would have to double all roles with a one for the disabled state. And when you're at it don't forget that there is a focused/non-focues state (see QPalette) too.

One simple approach I may propose - to ignore colors provided by ForegroundRole/BackgroundRole if widget is disabled.

My data is such important that it must have a red background even the widget is disabled.

StarterKit

@Christian-Ehrlicher ok, your point is clear and I agree it might be the case sometimes.
But from my side - I'm quite satisfied with options provided by Foreground and Background roles of model's data() method and I'm not happy to go back creating a lot of delegates here and there just to make a somewhat good-looking UI. And a big part of Qt is about UI so I disagree that this dull job should be handled this way. This is not to agrue, just to supply another point of view.

Saying this I'm thinking about anything in between - if I subclass QTableView can I adjust common colors somewhere before painting? will paintEvent help me with it? I see in manual that it is possible to call setBackgroundRole() and setPalette() - how do you think, is it possible to substitute colors in paintEvent handler? Or I need to go deeper and modify some of parent's methods? Any ideas?

Christian Ehrlicher

@StarterKit said in QTableView with custom model: color of disabled items:

Any ideas?

Use a delegate or a QIdentityProxyModel.

StarterKit

@Christian-Ehrlicher pardon, but I don't understand how QIdentityProxyModel can help for my case. From what I I read I see it returns the same values from data() method as any other model.

With regards to delegates - I already commented.

Christian Ehrlicher

@StarterKit said in QTableView with custom model: color of disabled items:

With regards to delegates - I already commented.

But you want to fiddle around in QTableView? Wow...

QIdentityProxyyModel

This ~~function~~ class is there to allow to modify values returned from data() without affecting the base model. So tell this model the current widget state and return the right colors from there.

JonB

@StarterKit
I replied earlier stating the only two (reasonable) choices are a delegate or a proxy model.

Since you do not wish to write a delegate, I suggested:

If the OP really does not wish to create a delegate for the QTableView, an alternative is to create a QAbstractProxyModel local to the QTableView. Interpose that between the view and the original source model, and do the required data() alteration there (where it can access the view to see if it is disabled). At least then he is sure the model returning the "manipulated" value for the color is totally local to the individual view.

So "do the required data() alteration there". A QIdentityProxyModel starts out "doing nothing" other than mapping straight through to the source model unchanged. You should derive from that to create your own sub-class. Then override and alter its data() method only (nothing else) to respond to the ForegroundRole by testing the view's "disablement" status and returning your desired color in that case. For all other cases in data() return the base QIdentityProxyModel's value.

StarterKit

Ok, guys. Thank your for this discussion. I'll go and change my models...