Review Request 116047: Add a space info observer, to share the space information between all views. Replaced the space information bar by a simple space information text.

[Emmanuel Pescosta]

> On Feb. 25, 2014, 4:51 p.m., Frank Reininghaus wrote:
> > First of all, thanks for looking into this.
> > 
> > To be honest, I'm not sure if mixing multiple unrelated changes in one patch is a good idea (even if we ignore for a moment that it's not clear at all if people will like the GUI change - my assumption is that most users who use the 'free space' bar won't). Such patches tend to hide subtle regressions.
> > 
> > If I'm not mistaken, one thing that might not be obvious when having a quick look at your patch is that it will query the free space for all paths shown in all Dolphin windows every 10 seconds, even if the computer is not used at all, and the screen is locked (the current code stops the timer if a view loses focus - the bug is that the timer is not restarted again). It gets worse if there are multiple Dolphin windows whose 10-second timers are not in sync.
> > 
> > This will waste energy and kill the batteries of laptop users.
> > 
> > Please discuss such ideas on the mailing list before changing lots of code!
> 
> Frank Reininghaus wrote:
>     BTW, I agree that having a shared cache for the "free space" in all views might be good (I also had this idea recently), but it must be implemented such that it's possible for hidden views to make the cache stop querying the free space of their mount points periodically.

Ok thanks for your feedback.

> even if we ignore for a moment that it's not clear at all if people will like the GUI change
Too bad, ok I'll remove it and make the shared cache better.

Do you have an idea, how we can solve this in an elegant way?

Maybe an "updater" object (unique pointer), when the view becomes visible it requests an updater 
object and when it goes out of scope again the updater object will be destroyed automatically.

The updater object, holds the timer and the mount point.

Code to request an updater:
std::unique_ptr<SpaceInfoUpdater> SpaceInfoObserver::createUpdater(const QString& mountPoint) const 
{
    std::unique_ptr<SpaceInfoUpdater> updater(new SpaceInfoUpdater(mountPoint));
    connect(updater, SIGNAL(updateMountPoint(QString)), this, SLOT(updateMountPoint(QString)));
    return updater;
}

updateMountPoint(QString) is a private slot in SpaceInfoObserver.

What do you think?


- Emmanuel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116047/#review50832
-----------------------------------------------------------


On Feb. 25, 2014, 4:19 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116047/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 4:19 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 318395 and 327708
>     http://bugs.kde.org/show_bug.cgi?id=318395
>     http://bugs.kde.org/show_bug.cgi?id=327708
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> 1.)
> Add a space info observer, to share the space information between all views.
> The updating of space information in split view is also fixed with this 
> patch. A KUniqueApplication based Dolphin version will also benefit from these
> changes ;)
> 
> 2.)
> Replaced the space information bar by a simple space information text.
> Benefits: We have more space for relevant information and it 
> looks nicer (personal opinion).
> 
> 
> I decided to fix these two bugs with one review request, because splitting
> this up into independent patches is much more work (space observer would need 
> more than just available space info, statusbar would also need a lot more code) 
> and I think it'll be harder to review.
> 
> 
> Btw.:
> If you don't like the space info as text idea, we can extend the free space
> observer, so that it works together with the current space info bar. ;)
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 3f58479 
>   dolphin/src/statusbar/dolphinstatusbar.h b2afe2e 
>   dolphin/src/statusbar/dolphinstatusbar.cpp 671ef4f 
>   dolphin/src/statusbar/spaceinfoobserver.h PRE-CREATION 
>   dolphin/src/statusbar/spaceinfoobserver.cpp PRE-CREATION 
>   dolphin/src/statusbar/statusbarspaceinfo.h 1849462 
>   dolphin/src/statusbar/statusbarspaceinfo.cpp 61b2833 
> 
> Diff: https://git.reviewboard.kde.org/r/116047/diff/
> 
> 
> Testing
> -------
> 
> Works fine for me.
> 
> 
> File Attachments
> ----------------
> 
> Dolphin Status Bar Old
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/25/7b5f8dc8-36fc-4751-b1d6-3861af6cc4e3__dolphin_statusbar_old.png
> Dolphin Status Bar New
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/02/25/40f28466-590a-4322-b8f4-50f9ca785b3a__dolphin_statusbar_new.png
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140225/d4cdf6a0/attachment.htm>