Did you test whether unpolish is actually called ? Last time I tried, it was not, at least not always. (depending on whether you run a QApplication, KApplication, etc.)
This is what led to the mess of destroying the singleton style in the plugin helper, which in turn created other crashes, and which was removed aferwards.
This all story of QStyle dinitialization is a mess ...

In D3240#60257, @hpereiradacosta wrote:

Did you test whether unpolish is actually called ? Last time I tried, it was not, at least not always. (depending on whether you run a QApplication, KApplication, etc.)
This is what led to the mess of destroying the singleton style in the plugin helper, which in turn created other crashes, and which was removed aferwards.
This all story of QStyle dinitialization is a mess ...

Ah
sorry.
I misread the fact that you actually make sure that kwin calls the unpolish.

In D3240#60265, @hpereiradacosta wrote:

In D3240#60257, @hpereiradacosta wrote:

Did you test whether unpolish is actually called ? Last time I tried, it was not, at least not always. (depending on whether you run a QApplication, KApplication, etc.)
This is what led to the mess of destroying the singleton style in the plugin helper, which in turn created other crashes, and which was removed aferwards.
This all story of QStyle dinitialization is a mess ...

Ah
sorry.
I misread the fact that you actually make sure that kwin calls the unpolish.

For the general case you are right. Unpolish is only called from QApplication::setStyle when the old style gets destroyed. On Application tear-down it's not called.

For the general case you are right. Unpolish is only called from QApplication::setStyle when the old style gets destroyed. On Application tear-down it's not called.

To be honest, I am a bit uneasy about this change. Sounds somewhat like abusing the API (since you then call the method explicitly in kwin). What if people later on start adding other "destruction" stuff in unpolish, unaware that it is actually not called ?

Maybe something cleaner would be to move everything from the destructor to unpolish (with proper reassignment to nullptr), and then call unpolish explicitely in the destructor.
The whole thing with tons of comments/warning ?

In D3240#60309, @hpereiradacosta wrote:

For the general case you are right. Unpolish is only called from QApplication::setStyle when the old style gets destroyed. On Application tear-down it's not called.

To be honest, I am a bit uneasy about this change. Sounds somewhat like abusing the API (since you then call the method explicitly in kwin). What if people later on start adding other "destruction" stuff in unpolish, unaware that it is actually not called ?

Maybe something cleaner would be to move everything from the destructor to unpolish (with proper reassignment to nullptr), and then call unpolish explicitely in the destructor.
The whole thing with tons of comments/warning ?

I don't like calling virtual methods from a dtor, always a mess. But we can do a private cleanup method and call it from both dtor and unpolish. Would that be OK to you?

I don't like calling virtual methods from a dtor, always a mess.

agreed

But we can do a private cleanup method and call it from both dtor and unpolish. Would that be OK to you?

yes

In D3240#61517, @ivan wrote:

I've just had a crash in Cantata (on launch) with this patch. The cleanup() is called which deletes the _shadowHelper which is later used in polish().

@ivan can you run cantata through gdb and break in the cleanup? I don't understand how it could have got there and to me it doesn't happen.