QIconEngine ownership regression in PyQt5 5.12

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

QIconEngine ownership regression in PyQt5 5.12

Ales Erjavec
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

Best regards
AleŇ°
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: QIconEngine ownership regression in PyQt5 5.12

Phil Thompson-5
On 14 Mar 2019, at 1:30 pm, Ales Erjavec <[hidden email]> 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
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: 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
<[hidden email]> wrote:

>
> On 14 Mar 2019, at 1:30 pm, Ales Erjavec <[hidden email]> 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
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt
Reply | Threaded
Open this post in threaded view
|

Re: QIconEngine ownership regression in PyQt5 5.12

Phil Thompson-5
On 19 Mar 2019, at 12:35 pm, Ales Erjavec <[hidden email]> wrote:
>
> Maybe the ownership could be transferred to the C++ (sip)QIconEngine itself?

That's what the previous buggy code actually did.

> 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

sip.transferto(engine, None) is better.

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

That's because QIconEngine has a virtual dtor which is used to detect when C++ invokes it.

> ## 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).

It is very unusual (I can't think of another case) where a class with value type semantics can take ownership of another object. sip does not have explicit support for this pattern so I'll add handwritten code to do the right thing.

Thanks,
Phil
_______________________________________________
PyQt mailing list    [hidden email]
https://www.riverbankcomputing.com/mailman/listinfo/pyqt