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

List:       kde-core-devel
Subject:    Re: Review Request: Displaying Job Progress in Icons - Review Request
From:       Fredrik_Höglund <fredrik () kde ! org>
Date:       2009-11-30 0:05:50
Message-ID: 20091130000550.24002.44979 () localhost
[Download RAW message or body]


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


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