[PyQt] QIconEngine ownership regression in PyQt5 5.12

[Ales Erjavec]

Maybe the ownership could be transferred to the C++ (sip)QIconEngine itself?

If I construct the QIcon with:

## code

def qicon_transfer(engine):
    # type: (QIconEngine) -> QIcon
    import sip
    icon = QIcon(engine)
    sip.transferto(engine, engine)
    return icon

## /code

Then the engine is preserved until the last icon is deleted.

## code

def test_icon_transfer():
    from weakref import ref
    eng = IconEngine("a", QColor("red"))
    engref = ref(eng)

    icon = qicon_transfer(eng)

    del eng
    assert engref() is not None
    icon1 = QIcon(icon)

    del icon
    assert engref() is not None
    icon2 = QIcon(icon1)

    del icon1
    assert engref() is not None

    del icon2
    assert engref() is None

## /code

There should be no issue with the transfer back because there is no
way to get the engine out of the QIcon.

I think this preserves the Qt's design. QIcon has value type semantics
and practically all API's that accept a QIcon do so by value (copy construct).

Best wishes,
Aleš

On Mon, Mar 18, 2019 at 3:53 PM Phil Thompson
<phil at riverbankcomputing.com> wrote:
>
> 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