Review Request: Displaying Job Progress in Icons - Review Request

Sat Nov 28 18:25:09 GMT 2009

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


/branches/work/sok-progress-in-icons/kdelibs/kdeui/jobs/kuiserverjobtracker.cpp
<http://reviewboard.kde.org/r/2057/#comment2601>

    For compatibility reasons, I think the tracker should use JobView, not JobViewV2 everywhere it can. Only in the place where a V2-specific call must be made, it should downcast and call - assuming that the dbus object implements the V2 interface, of course.
    
    Otherwise, people upgrading kdelibs and not kdebase yet (or not having restarted plasma yet) will experience trouble (although I'm not exactly sure which kind of trouble; if it's just the setDestUrl call that fails that's fine, but maybe nothing works if it doesn't recognize the V2 interface name)


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

    ItemJobRole sounds like it will return the job for the item. If it's only a bool, it should be named HasJobRole for instance.


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

    (view != delegate, so you mean the delegate, not the view).
    
    But what about letting the delegate call the method in the model, then?
    Although, this would be only when value==true, so the model would never be reset to the default (not updating the job-progress data), since it can't know how many delegates are still interested in that info.
    I don't suppose this is a big issue though, in practice.


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

    Add a comment here. This is the list of URLs currently being copied to, right?
    For clarity, maybe name this
    allCurrentDestUrls. All as in: all the transfers happening right now, not only those related to this KDirModel.


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

    QStringList -> const QStringList&


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

    Ouch, this has to go over the whole hash. Better solution:
    
    const QString url = node->item()->url();


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

    simplify with return QVariant(d->destUrls.contains(url)) ?


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

    Add Qt::UniqueConnection as 5th argument, just in case this is called twice.


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

    For a more Qt-like method name, setJobTransfersVisible?
    setJobProgressVisible?


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

    Fredrikh should probably review the actual drawing code.


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

    QColor(QColor(..)) not useful, remove one.


/branches/work/sok-progress-in-icons/kdelibs/kio/kio/org.kde.kuiserver.xml
<http://reviewboard.kde.org/r/2057/#comment2612>

    jobUrls sounds like a getter. Well, indirectly it's one, but asynchronously. Maybe rename to emitJobUrls?


- David


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