Port KAnimatedSystemTray into KSystemTray

[Aaron J. Seigo]

On Saturday 13 September 2008, Ingo Klöcker wrote:
> On Saturday 13 September 2008, Aaron J. Seigo wrote:
> > suggestion: reparent the movie to the tray icon
> >
> > rationale: instead of deleting the movie member directly, reparent it
> > in setMovie: movie->setParent(this); not only does this make the
> > memory management a bit easier (less code), this will prevent crashes
> > where people do something like:
> >
> > new KSystemTrayIcon(new QMovie(pathToMovie, someObject), this);
> >
> > and then later delete someObject, causing the movie to be deleted as
> > well which would then lead to a crash in KSystemTrayIcon
>
> Wouldn't it make sense to use QPointer<QMovie> instead of QMovie* for
> the member variable? This would prevent a crash in KSystemTrayIcon if
> somebody does
>
>   QMovie *movie = new QMovie(pathToMovie, someObject);
>   new KSystemTrayIcon(movie, this);
>
> and later
>
>   delete movie;

another approach is to connect to the objectDestroyed signal on the movie and 
null the pointer ... but yeah, i like the QPointer approach; it still doesn't 
prevent people from being extra stupid, of course. but one can only protect 
people so much.

personally, i would just call any manual deletion of a movie object passed to 
a system tray icon a bug in the application and not worry too much about it. 
maybe that's just me being too lazy and trusting though.

i was more worried about the lack of reparenting because that's something that 
is easy to miss and is "magic" in the code: parenting implies later automatic 
deletion. so it's an easier bug to stumble into compared to manually deleting 
an object.

still ... safety is good.

> BTW, the API docs of setMovie() lack the note "Memory management for the
> movie will be handled by KAnimatedSystemTrayIcon."

+1

> Is copying QMovie expensive or is it implicitely shared? If it isn't
> expensive to copy a QMovie then I suggest to pass a const QMovie& to
> the c'tor and to setMovie() instead of a QMovie*. Apart from avoiding
> problems with memory management this would also make the signature of
> the QMovie-taking c'tor and of setMovie() equivalent to the signature
> of the c'tor taking a QIcon and of setIcon() (given this setter exists,
> that is).

as Alex noted, it's a QObject so inherently not copyable. there are other ways 
around it (creating a new QMovie object inside the class based on some data 
passed in, or whatever) but those all look uglier in the end imho.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080913/22b9845f/attachment.sig>