[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: [Panel-devel] engines, applets and timing
From:       "Aaron J. Seigo" <aseigo () kde ! org>
Date:       2007-09-16 15:36:37
Message-ID: 200709160936.43834.aseigo () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Sunday 16 September 2007, Michael Olbrich wrote:
> 1)
> uint timeSinceLastUpdate() const;
> to
> int timeSinceLastUpdate() const;
>
> it avoids signed/unsigned comparison and internally it's an int anyway.

hm. ok ... even though we can't have negative times here =) internally it was 
a uint until i changed what i was storing there, but changing the API to 
reflect what is there now is fine =)

> 2)
> d->relays.erase(d->relays.find(relay->m_interval));
> to
> d->relays.remove(relay->m_interval);
>
> in this case it's the same but nicer.

sure =)

> 3)
> in DataContainer::connectVisualization d->relayObjects[visualization]
> was not set for (updateInterval >=3D1)

are you sure? it's set in DataContainer::Private::signalRelay, is it not?

> and related to that fix:
> QObject* DataContainer::Private::signalRelay
> to
> SignalRelay* DataContainer::Private::signalRelay
>
> to avoid casting

there is a reason the SignalRelay class is in a _p.h file ;) external usage of 
this class should not need to know about SignalRelay at all

> 4)
> it's not possible to disconnect during QObject::destroyed()

sure it is; well at least it doesn't crash doing so ... and the real issue for 
calling disconnectVisualization on destroyed() is to remove the SignalMapper 
from our data structures. QObject disconnect()s automatically on destruction 
(obviously) but DataContainer needs to remove it from its record keeping

> checking the sender is a bit ugly but I didn't feel like writing an
> extra slot?

except that now you've made it impossible to call disconnectVisualization 
directly. this part of the patch is wrong.

> 5)
> DataEngine::setupdateInterval
> should be
> DataEngine::setUpdateInterval
>
> right?

whoops, yes... silly capital letters.

> 6) (not yet implemented)
> I would like to change minUpdateFreq to minUpdateInterval because that's
> what it acually is.

+1


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

[Attachment #5 (application/pgp-signature)]

_______________________________________________
Panel-devel mailing list
Panel-devel@kde.org
https://mail.kde.org/mailman/listinfo/panel-devel


[prev in list] [next in list] [prev in thread] [next in thread]