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.
Frank Reininghaus
frank78ac at googlemail.com
Mon Mar 3 09:57:57 GMT 2014
> On Feb. 25, 2014, 3: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.
>
> Emmanuel Pescosta wrote:
> 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?
>
>
> Do you have an idea, how we can solve this in an elegant way?
I haven't tested it, but I think that the easiest solution for the "Dolphin doesn't update the space information" would be to remove the
if (!event->spontaneous())
check from StatusBarSpaceInfo::showEvent(QShowEvent* event). When the bug was filed, I noticed this check, see my comment in the bug report (basically, it ensures that the timer is only started when the "free space bar" is first shown, and not if it is hidden and then shown again). I was unsure though if there was a good reason for having this check that I overlooked, so I hesitated to change it.
But thinking about it again, I can't see how removing this check could cause any major problems (like polling the disk all the time while the window is minimized or on another desktop). If you agree that this makes sense, we could change it in KDE/4.13 to at least fix the bug.
About the shared cache thing: yes, I have some ideas. I'll try to come up with some code in the next few days.
- Frank
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116047/#review50832
-----------------------------------------------------------
On Feb. 25, 2014, 3: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, 3: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/20140303/ec68e3f5/attachment.htm>
More information about the kfm-devel
mailing list