Review Request for KDecoration

[Thomas Lübking]

On Dienstag, 4. November 2014 10:20:18 CEST, Martin Gräßlin wrote:
>> DecorationSettings
>> onAllDesktopsAvailableChanged
>> -> remove the "on" prefix
>
> No, maybe the name is bad, but if the on is removed the meaning 
> changes. It's 
> whether one can put DecoratedClients "on all desktops" (at lest two virtual 
> desktops). I'll think about a better name.

_____________

>> Generally: Ceterum censeo QList esse delendam
>
> :-D
>
>> -> I suggest you remove QList and use QVector by default everywhere.
Reasoning?
QVector operates on adjacent memory - it's very fast if you've a "create once, read many" structure of "reasonable" size, but when the size varies a lot and/or glib has trouble to allocate the amount of adjacent memory, things will go south.

Now, this pretty much is the case for the QList's used in this contex (so replacing them with QVector is certainly an improvement), but I'd be very careful about extrapolating your personal Cannae here ;-)


> No, the point I wanted to make by using a QPointer is that the API doesn't 
> guarantee that the returned pointer stays valid. Kind of I want to enforce 
> correct usage (note this are plugins loaded into KWin, I don't 
> want a plugin 
> to crash KWin by accessing a dangling pointer. Better save in the API than 
> sorry).

I'd rather stress "notice that the return value can be nullptr and change during the lifetime of the DecoratedClient".

The user can still do
Decoration *m_deco = m_client->decoration(); // fails at some point

Overmore, the guarded pointer may turn nullptr, but m_client->decoration() return a valid pointer again, right (deco changed)?

So the strategy you've to sell is to NOT store the pointer locally - and QPointer does actually the opposite, does it?

>> ::findBridge
> could you please explain why or point me to a web resource describing it?
Make it invisible. Any local helper function, not (supposed to be) used outside the source file probably should be tagged "static".


Cheers,
Thomas