KCapacitybar on places item view

[Kevin Ottens]

Le Wednesday 06 August 2008, Rafael Fernández López a écrit :
> This one is pretty strange and sounds like my first trying on this... maybe
> I generated wrongly the patch, or you had some garbage on it. Please,
> before applying this patch (note you will need patch -p1, since it is
> generated with git), make sure your local folder is clean.
>
> I am sure this is something wrong with the patch or applying it, or if you
> did some special combination of actions, please specify. I am sure it is
> pretty impossible to get this with the current code.

It was, I tried the old patch again but updated kdeui and kfile and got the 
same result. With the new patch you posted this morning it's fine though.

> > 2) Please don't move the KFilePlacesView::Private class inside the cpp
> > file. The current layout is perfectly intended, I'd like to avoid
> > coupling the delegate with the view class as much as possible. The idea
> > being that at one point we could make the delegate more generic and help
> > get the kind of animations available in the places delegate to other
> > places. The coupling is already bad as it is, this move and the friend
> > directive makes it worse. ++beer from me if you avoid it.
>
> Avoided. I moved the maps to the delegate, and added some accessors
> (probably their names are not good, but you know, that is not my strongest
> ability). Suggestions for them ?

They're all serving a given purpose which is the capacity bar fading. So 
really I'd say that:
+    void insertTimeLineMap(const QModelIndex &index, QTimeLine *timeLine);
+    void removeTimeLineMap(const QModelIndex &index);
+    QModelIndex indexForTimeLine(QTimeLine *timeLine) const;
+    QTimeLine *timeLineForIndex(const QModelIndex &index) const;
Should be:
+    void addFadeAnimation(const QModelIndex &index, QTimeLine *timeLine);
+    void removeFadeAnimation(const QModelIndex &index);
+    QModelIndex indexForFadeAnimation(QTimeLine *timeLine) const;
+    QTimeLine *fadeAnimationForIndex(const QModelIndex &index) const;

> > 3) _k_timeLineValueChanged() should probably have another name. It's not
> > the only time line used in there, so please make it more specific.
>
> Fixed. Called now _k_capacityBarTimeLineValueChanged().

_k_capacityBarFadeValueChanged()?

> > 4) _k_slotPollDevices() should be renamed too. We don't use the "slot"
> > convention in names for this class.
>
> Fixed. Called now _k_pollDevices().

_k_triggerDevicePolling()? (AFAIK it doesn't really poll, just does the 
necessary so that the device polling will happen "soon enough")

> > 5) The indentation in KFilePlacesEventWatcher event filter should be
> > modified to avoid the two aligned } at the end of each case block.
>
> Fixed.
>
> The patch can be found at:
> http://media.ereslibre.es/2008/08/kdelibs.diff
>
> Ok to commit ?

Please consider the proposed renames first. Other than that it seems ok to 
commit (although keep in mind that because of the patch size I might have 
overlooked some potential issues).

Regards.
-- 
Kévin 'ervin' Ottens, http://ervin.ipsquad.net
"Ni le maître sans disciple, Ni le disciple sans maître,
Ne font reculer l'ignorance."
-------------- 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/20080806/0f4313b3/attachment.sig>