Review Request: Displaying Job Progress in Icons - Review Request

Mon Nov 30 00:05:50 GMT 2009

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2057/#review3338
-----------------------------------------------------------

I'm not really happy about all the magic numbers in the painting code, but other than that it looks fine.
Do those numbers work with icon themes other than Oxygen?

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/delegateanimationhandler.cpp
<http://reviewboard.kde.org/r/2057/#comment2663>

    This will stop other animations tracked by the state.

    Use animating |= index.model()->data(index, KDirModel::ItemJobRole).toBool() instead.

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/delegateanimationhandler.cpp
<http://reviewboard.kde.org/r/2057/#comment2664>

    hasJobAnimation()

    The animation state can track multiple animations.

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.h
<http://reviewboard.kde.org/r/2057/#comment2667>

    There's no getter or Q_PROPERTY for this function.

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.cpp
<http://reviewboard.kde.org/r/2057/#comment2665>

    Whitespace.

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.cpp
<http://reviewboard.kde.org/r/2057/#comment2662>

    This will break all other animations for files that are being downloaded.

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.cpp
<http://reviewboard.kde.org/r/2057/#comment2668>

    If you move this code into a function, you don't have to disable the other animations while an item is being downloaded. You can call it both at the bottom of paint(), and after the cached rendering has been drawn.

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.cpp
<http://reviewboard.kde.org/r/2057/#comment2669>

    if (index.data(KDirModel::ItemJobRole).toBool()) {

/branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.cpp
<http://reviewboard.kde.org/r/2057/#comment2666>

    This will reload the icon on every frame.

- Fredrik

On 2009-11-12 21:27:08, Shaun Reich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2057/
> -----------------------------------------------------------
> 
> (Updated 2009-11-12 21:27:08)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Hello,
> 
> This merge request is for the changes I have done (on my branch), along with other changes(but those are just waiting for this review).
> 
> The branch was for a project that I have done; displaying the job progress on the file/folder icons. It essentially shows the status, as in whether or not there is a job being performed on a certain dest. url, if there is, a download icon will be shown at the corner, with an animation(circles rotating around a focal point), to show that a transfer is happening (download, copy, etc..). 
> 
> It should be pretty subtle, but may need some tweaking.
> 
> 
> I don't really like the KDirModel::setDisplayJobTransfers(bool), and KFileItemDelegate::setDisplayJobTranfers(bool). They sound terrible imo, and seem to go against Nice API Design, but I use them for lack of a better name. Hopefully you guys have some suggestions?
> 
> Naturally those 2 method properties are off by default, the individual apps should turn it on at their discretion. I will probably do that for Dolphin and the Folderview plasmoid.
> 
> 
> There was some oddity that I experienced, that seemed like an impossibility (then again, lots of bugs do). It wouldn't stop displaying that a job was happening, but I tracked it down and if anything it would have been the sender (kuiserver)'s issue. But I've tested kuiserver quite rigorously...I'm thinking it is just some problem dealing with the *major* version differences I am running.. *shrug*
> 
> 
> I can no longer test this branch with my system, as I run trunk and running this really old checkout side by side...well, that was a pain I am not going to go through again, as apps were crashing so often etc.. and I *still* don't have my plasma's desktop activities back up and running, only 2 system monitors on the desktop :)
> 
> 
> Once it's committed to trunk, I should be able to quickly verify and track down any issues...
> 
> 
> Diffs
> -----
> 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/delegateanimationhandler_p.h 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/delegateanimationhandler.cpp 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/copyjob.cpp 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/CMakeLists.txt 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kdeui/jobs/org.kde.JobViewV2.xml PRE-CREATION 
>   /branches/work/sok-progress-in-icons/kdelibs/kdeui/CMakeLists.txt 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kdeui/jobs/kuiserverjobtracker.cpp 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/job.cpp 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/joburlcache.h PRE-CREATION 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/joburlcache.cpp PRE-CREATION 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/kdirmodel.h 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/kdirmodel.cpp 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.h 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/kfileitemdelegate.cpp 978718 
>   /branches/work/sok-progress-in-icons/kdelibs/kio/kio/org.kde.kuiserver.xml PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/2057/diff
> 
> 
> Testing
> -------
> 
> oodles.
> 
> 
> Thanks,
> 
> Shaun
> 
>