[PyQt] QIconEngine ownership regression in PyQt5 5.12

[Phil Thompson]

On 14 Mar 2019, at 1:30 pm, Ales Erjavec <ales.erjavec324 at gmail.com> wrote:
> 
> Hi,
> 
> There seems to be a regression in PyQt5 5.12 in QIconEngine ownership
> transfer to QIcon.
> I.e.
> 
> ### code
> icon1 = QIcon(IconEngine(...))
> icon2 = QIcon(icon1)
> del icon1 # the IconEngine is deleted along with the icon1
> ### /code
> 
> After this icon2 no longer renders properly. The QIconEngine's are
> implicitly shared by QIcon
> instances so the engine should not be deleted with icon1.
> 
> This works as expected in in PyQt5 5.11, 5.10, 5.9
> 
> A more complete example with an icon engine implementation:
> 
> ### code
> 
> from PyQt5.QtCore import Qt, QRect
> from PyQt5.QtGui import (
>    QIcon, QIconEngine, QPainter, QColor, QPixmap, QStandardItem,
>    QStandardItemModel
> )
> from PyQt5.QtWidgets import QApplication, QListView
> 
> 
> class IconEngine(QIconEngine):
>    """A simple QIconEngine drawing a single text character."""
>    def __init__(self, char, color):
>        # type: (str, QColor) -> None
>        super().__init__()
>        self.char = char
>        self.color = QColor(color)
> 
>    def paint(self, painter, rect, mode, state):
>        # type: (QPainter, QRect, QIcon.Mode, QIcon.State) -> None
>        size = rect.size()
>        if size.isNull():
>            return
>        dpr = painter.device().devicePixelRatioF()
>        size = size * dpr
>        painter.drawPixmap(rect, self.pixmap(size, mode, state))
> 
>    def pixmap(self, size, mode, state):
>        # type: (QSize, QIcon.Mode, QIcon.State) -> QPixmap
>        pm = QPixmap(size)
>        pm.fill(Qt.transparent)
>        painter = QPainter(pm)
>        painter.setRenderHints(QPainter.TextAntialiasing)
>        size = size.width()
>        painter.setPen(self.color)
>        painter.setBrush(self.color)
>        margin = 1 + size // 16
>        text_margin = size // 20
>        rect = QRect(margin, margin, size - 2 * margin, size - 2 * margin)
> 
>        font = painter.font()
>        font.setPixelSize(size - 2 * margin - 2 * text_margin)
>        font.setBold(True)
> 
>        painter.setFont(font)
>        painter.drawText(rect, Qt.AlignCenter, self.char)
>        painter.end()
>        return pm
> 
>    def clone(self):
>        return IconEngine(self.char, self.color)
> 
>    def __del__(self):
>        print("del")
> 
> def main(argv=[]):
>    app = QApplication(argv)
>    view = QListView()
>    model = QStandardItemModel()
>    item1 = QStandardItem("R")
>    item1.setIcon(QIcon(IconEngine('R', QColor("red"))))
>    item2 = QStandardItem("B")
>    item2.setIcon(QIcon(IconEngine('B', QColor("blue"))))
>    item3 = QStandardItem("G")
>    item3.setIcon(QIcon(IconEngine('G', QColor("green"))))
> 
>    model.appendRow([item1])
>    model.appendRow([item2])
>    model.appendRow([item3])
> 
>    view.setModel(model)
>    view.show()
> 
>    return app.exec_()
> 
> 
> if __name__ == "__main__":
>    import sys
>    sys.exit(main())
> 
> ### /code
> 
> In PyQt5 5.12 the __del__ method is called immediately after setting
> the icon on the QStandardItem (and the icons do not render in the
> view)
> In PyQt5 5.11, 5.10, 5.9 it is called after main finishes (and icons
> render correctly in the view).
> 
> Tested on macOS 10.14.5 with Python 3.6.6 and 3.7.0

PyQt doesn't know that a QIconEngine is explicitly shared between copies fo a QIcon so the latest behaviour is what I would expect. You need to keep an explicit reference to either the QIconEngine or the QIcon itself.

The reason this worked before was due to a bug in the implementation of /Transfer/ when applied to a ctor argument. While the reference count was increased, the association of the argument (the QIconEngine) to the thing being constructed (the QIcon) was not made. Therefore when the QIcon was garbage collected the QIconEngine was not. The bug was fixed in SIP v4.19.14.

Phil