After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 573980 - Low Disk Space warning doesn't suggest emptying the Trash if that will help
Low Disk Space warning doesn't suggest emptying the Trash if that will help
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: plugins
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2009-03-03 21:03 UTC by Matthew Paul Thomas (mpt)
Modified: 2009-08-08 14:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to implement http://live.gnome.org/LowDiskSpaceWarning (74.96 KB, patch)
2009-06-28 12:17 UTC, Chris Coulson
reviewed Details | Review
This is what the warning looks like (with trash) (953.02 KB, image/png)
2009-06-28 12:29 UTC, Chris Coulson
  Details
This is what the warning looks like (without trash) (961.00 KB, image/png)
2009-06-28 12:30 UTC, Chris Coulson
  Details
Patch to implement http://live.gnome.org/LowDiskSpaceWarning (74.41 KB, patch)
2009-07-18 17:32 UTC, Chris Coulson
committed Details | Review
Diff between patches (8.29 KB, text/plain)
2009-07-18 17:35 UTC, Chris Coulson
  Details

Description Matthew Paul Thomas (mpt) 2009-03-03 21:03:51 UTC
The low disk space warning implemented in bug 557647 lets you launch Baobab, if it is installed, to examine how the disk space is being used. This is fine and nifty.

The simplest solution to a problem of low disk space, however, is to empty the Trash. So if (and only if) the nearly-full partition has any items in the Trash, the warning should include emptying the Trash in its suggestions, and offer a button for doing that.

Mini-spec: <http://live.gnome.org/LowDiskSpaceWarning>
Comment 1 Mitesh Sharma 2009-04-04 06:02:50 UTC
(In reply to comment #0)
> The low disk space warning implemented in bug 557647 lets you launch Baobab, if
> it is installed, to examine how the disk space is being used. This is fine and
> nifty.
> 
> The simplest solution to a problem of low disk space, however, is to empty the
> Trash. So if (and only if) the nearly-full partition has any items in the
> Trash, the warning should include emptying the Trash in its suggestions, and
> offer a button for doing that.
> 
> Mini-spec: <http://live.gnome.org/LowDiskSpaceWarning>
> 

I would love to work in this asap since i was asked to submit a patch for my GSoC application. I would be kind of you to help me through
Comment 2 Jens Granseuer 2009-04-05 09:05:35 UTC
What exactly do you need/expect? Matthew has summed it up pretty nicely on the wiki, the g-s-d plugin code is in svn under gnome-settings-daemon/trunk/plugins/housekeeping, and I'm sure you can take a peek at the baobab and the trash applet (gnome-panel) sources to find out how work with disks and the trash.
Comment 3 Chris Coulson 2009-05-30 20:14:52 UTC
FWIW, I'm currently doing some work to implement Matthew's spec in Ubuntu, and the trash emptying will form part of that work.
Comment 4 Chris Coulson 2009-06-28 12:16:22 UTC
Here's an initial patch that implements the spec at http://live.gnome.org/LowDiskSpaceWarning (including the trash emptying - borrowed from gnome-applets and adapted slightly).

I had to change the structure of the code a little. The old code just iterated through every GUnixMountEntry and displayed a notification if it was low on space. The spec dictates that you know in advance if there are any other usable volumes, and it also states "When a non-removable, non-hotpluggable partition is running low on disk space (how low?), an alert box should appear unfocused to warn you of this and advise you what to do".

To do this, the new code builds a list of static mounts using GUnixMountPoint. It then maps these in to a corresponding GUnixMountEntry if the GUnixMountPoint is mounted. I couldn't figure out any other way of doing this. The effect of this is that it ignores dynamically mounted media, which I think is what the spec intended.

It then iterates through the list of GUnixMountEntry's, checking the space available. Once this has happened for every mount, it displays the warning(s).

The dialog also has a checkutton to disable future warnings, as requested here: https://bugs.edge.launchpad.net/ubuntu/+bug/390504

I also made the thresholds configurable via GConf. Some of the comments already in the code suggested that was already planned, but I had to implement it to make the testing of it a bit easier.

As it's quite a big patch, I've also hosted the code on http://github.com/chrisccoulson/gnome-settings-daemon/tree/housekeeping_ldsm_notification
Comment 5 Chris Coulson 2009-06-28 12:17:20 UTC
Created attachment 137490 [details] [review]
Patch to implement http://live.gnome.org/LowDiskSpaceWarning
Comment 6 Chris Coulson 2009-06-28 12:29:28 UTC
Created attachment 137491 [details]
This is what the warning looks like (with trash)
Comment 7 Chris Coulson 2009-06-28 12:30:40 UTC
Created attachment 137492 [details]
This is what the warning looks like (without trash)
Comment 8 Milan Bouchet-Valat 2009-06-28 13:21:37 UTC
While you're working on this, would you mind polishing it even further? Here are a few ideas:
- show the size of files in the Trash, and don't suggest emptying the Trash if it's empty. You could go even further and compute whether emptying the Trash will be sufficient to get above the threshold; if not, then removing files or programs will be required. This can be used to customize the message, e.g. if the Trash is gigantic, you can just tell the user to empty it, without speaking of removing files and programs at all.

- detect critical system mounts and give an explanation about the problems that may arise. If /, /var/, /home/ are full, the system can run into trouble quickly, while custom mounts in /mnt/ are not really an issue. Is it even useful to warn about non-system mounts being full?

- the icon for the "Ignore" button is a little aggressive for what it does. That doesn't seem really consistent with other uses of such icons in the desktop. I'd rather use something like gtk-close or gtk-quit.


But overall I really like that feature!
Comment 9 Chris Coulson 2009-06-29 12:03:39 UTC
Thank you for your comments. I've written a quick response each of them in turn:

1) That would be quite interesting to implement. However, it could create a lot of up-front disk IO, as I'd need to stat() every file in the trash before displaying the dialog. That could take some time if a user doesn't empty their trash very often (think about how long it takes for Nautilus to display folder size when you open the properties window on a folder with lots of files). The current implementation will only display the "Empty Trash" button if there are files in the users trash folder for that particular mount.

2) I think that the only issue with this point would be "information overload". There is already quite a lot of text in the dialog, and adding more might make it confusing.

3) The icon for the ignore button is just a GTK_STOCK_CANCEL. i think it looks aggressive on my system due to the icon theme I use.

Comment 10 Colin Walters 2009-07-15 21:58:57 UTC
Looks like a high quality patch in general.

* This is totally minor, but i'd say "Free percentage notify threshold" instead of "Free percent".

+                *ignore_paths = g_slist_remove (*ignore_paths, found->data);

Are we leaking the mount point here?  We strdup it in the update case, but remember g_slist_remove won't g_free the data for you.

I glanced over the rest of the patch and didn't see any obvious superficial code issues, but I don't know the current code.

One thing I'm wondering - do you determine if hte user can write to the mounts?  

In general I think one of the gnome-settings-daemon maintainers should apply this patch.
Comment 11 Chris Coulson 2009-07-15 22:09:17 UTC
Thanks Colin,

I think that mount point does get leaked. I'll fix that when I get the chance. I did run it through valgrind when I wrote the patch, but I probably haven't tested every possible combination yet.

The patch currently does not check if the user can write to the mount - I wasn't sure of any non-hacky way to do that. In any case, if the user can't write to the mount, the mount shouldn't have any of their trash on it, so the only options displayed will be "Ignore" and "Examine".
Comment 12 Jens Granseuer 2009-07-18 10:57:50 UTC
Some additional comments:

-#include "config.h"

config.h must always be the first include. Don't remove this.

+                if (!g_file_test (trash_files_dir, G_FILE_TEST_EXISTS) || !g_file_test (trash_files_dir, G_FILE_TEST_IS_DIR)) {

Checking for just _IS_DIR is sufficient. It won't be if it doesn't exist. ;-)

+                        trash_dir = g_strdup_printf (".Trash-%s", uid);
+                        trash_files_dir = g_build_filename (path, trash_dir, "files", NULL);
+                        if (!g_file_test (trash_files_dir, G_FILE_TEST_EXISTS) || !g_file_test (trash_files_dir, G_FILE_TEST_IS_DIR)) {
+                                g_free (trash_files_dir);
+                                g_free (trash_dir);
+                                g_free (uid);
+                                return has_trash;
+                        }
+                        g_free (trash_dir);

If you free trash_dir right after using it to create trash_files_dir you'll 
save an instruction in the branch.

+        if (dialog->priv->other_partitions)
+                text = g_strdup (_("Don't show any warnings again for this filesystem"));
+        else
+                text = g_strdup (_("Don't show any warnings again"));

Since those texts are static it looks like you don't need to dup them.
Same with gsd_ldsm_dialog_get_secondary_text.
Comment 13 Chris Coulson 2009-07-18 17:32:29 UTC
Created attachment 138688 [details] [review]
Patch to implement http://live.gnome.org/LowDiskSpaceWarning

Thanks Jens / Colin,

Here is an updated version of the patch.
Comment 14 Chris Coulson 2009-07-18 17:35:04 UTC
Created attachment 138689 [details]
Diff between patches

And here are the changes I made between the first version and the newer one.
Comment 15 Jens Granseuer 2009-07-19 11:25:19 UTC
Thanks, Chris. Feel free to commit to trunk.

Since we're already in the string/UI announcement period it would be great if you could send the respective mails out (cf. http://live.gnome.org/ReleasePlanning/Freezes).
Comment 16 Jens Granseuer 2009-08-08 13:54:25 UTC
Already applied some time ago it seems. Somebody forgot to close this.
Comment 17 Chris Coulson 2009-08-08 14:37:08 UTC
That will be me - I'm not allowed to change the status