451599 - Add preferences UI for disk cache size and clearing the cache

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect, P2)

Description

[Mark Banner (:standard8)]

Bug 450456 added support for disk cache.

We want to also add support for allowing the user to change the size and clear the cache from the preferences UI.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

Place this under Disk Space
  Copy the firefox layout and place it under the current compact folders pref

Someone will have to remove the pref from bug 451620 to make some more room before I'd allow this addition.  I'm also looking at the Offline pref for removal as well.

Comment 2

[rsx11m]

Just wondering, how would a user specify that no disk cache is wanted?
Setting the size to zero should probably do it, and there is also the browser.cache.disk.enable preference.

Comment 3

[Ronald Killmer]

(In reply to comment #2)
> Just wondering, how would a user specify that no disk cache is wanted?
> Setting the size to zero should probably do it, and there is also the
> browser.cache.disk.enable preference.
> 

I believe the browser.cache.disk.enable is the pref which would be set by the new UI item. The Message pane is the "Browser" component IIUC from prior comments I have read, which IIRC were related to the RSS feeds. 

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #2)
> Just wondering, how would a user specify that no disk cache is wanted?
> Setting the size to zero should probably do it, and there is also the
> browser.cache.disk.enable preference.

Setting the value to zero seems like a reasonable method to set no cache for this kind of preference.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: WIP for adding the pref

Here's the patch for this, though it's untested as my TB src won't build today.  Just didn't want to lose this.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: v2 of patch adds cache pref UI

Only difference here is that I've swapped the Compact Folders pref to be on bottom because I didn't like how they lined up otherwise.  Will attach screenshot of change.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: screenshot of cache pref UI

here's the screenshot of the change

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

updating flags and status whiteboard.  need to find reviewer

Comment 9

[Ronald Killmer]

Look forward to testing this.

Comment 10

[Magnus Melin [:mkmelin]]

Without bug 439731 the actual use for this if of course a bit limited... but we should really try to get that fixed.

Hm, aren't those are really KiB/MiB, so 1024 not 1000? (Copied from ff, sure.) A tab snuck in too.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

> Hm, aren't those are really KiB/MiB, so 1024 not 1000? (Copied from ff, sure.)

*shrug* yeah, copied from what they were doing.

> A tab snuck in too.

damn!  new patch coming...

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: v3 cleans up white space

no changes to the screenshot needed, this just cleans up the white space problem

magnus: do you want to review this?

Comment 13

[Magnus Melin [:mkmelin]]

We should calculate it correctly, using 1024. I filed bug 457906 to get the calculation fixed in firefox.

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

if the person who is committing this wants to make that change to the current patch it's fine with me.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> if the person who is committing this wants to make that change to the current
> patch it's fine with me.

Preferably it is easier for the person committing if it is a clean patch - it can just be downloaded and go straight in.

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

Just noting something.  For this preference I don't think it's really going to make a difference about the 1024 vs. 1000 calculation because people probably won't see the space being compared in another UI.  Meaning that when you buy a 1.0G USB stick and then it shows up on your computer as 1.07GB or something else it's confusing; if the size isn't going to show up anywhere else then it probably just doesn't matter.

In general I've always felt that computer systems should stick with what the marketing change has been and use 1000 because that's what most people would understand.  Using MiB and KiB instead is just asking for pain and is sure to confuse.

A recent blog entry I saw has some excellent comments about the 1024 vs 1000 issue
http://blogs.gnome.org/cneumair/2008/09/30/1-kb-1024-bytes-no-1-kb-1000-bytes/

Comment 17

[Ronald Killmer]

(In reply to comment #16)
> In general I've always felt that computer systems should stick with what the
> marketing change has been and use 1000 because that's what most people would
> understand.  Using MiB and KiB instead is just asking for pain and is sure to
> confuse.
> 
> http://blogs.gnome.org/cneumair/2008/09/30/1-kb-1024-bytes-no-1-kb-1000-bytes/

Read the blog. The front end UI should honor the Users desire to set a number of 'Units'. What the backend needs is the programmers duty to compute if the hardware has mandatory allocation requirements.

Comment 18

[Magnus Melin [:mkmelin]]

It doesn't really help to use 1000, as the KBs are still in 1024, so you just end up with the odd mixed format. That odd format is only used by floppy drives.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 340948 [details] [diff] [review]
v3 cleans up white space

>+    var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
>+                         	       .getService(Components.interfaces.nsICacheService);

Still a tab here.

>+      <preference id="browser.cache.disk.capacity"     name="browser.cache.disk.capacity"     type="int"/>

This white spacing is odd, since it's new code, I'd go for no extra space at all, and wrap it to two lines if necessary.

Other than that and the change to 1024, I think it looks good though. 

For the future wishlist: it would be nice with some kind of feedback for pushing the clear button.

Comment 20

[Bryan Clark (DevTools PM) [:clarkbw]]

> >+    var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
> >+                         	       .getService(Components.interfaces.nsICacheService);
> 
> Still a tab here.

Sorry about that.  I should probably use a real programming editor if I'm going to keep making patches...

> >+      <preference id="browser.cache.disk.capacity"     name="browser.cache.disk.capacity"     type="int"/>
> 
> This white spacing is odd, since it's new code, I'd go for no extra space at
> all, and wrap it to two lines if necessary.

Sure.

> Other than that and the change to 1024, I think it looks good though. 

Ok, new version with those changes coming up.

> For the future wishlist: it would be nice with some kind of feedback for
> pushing the clear button.

Yeah, the silent operation could probably use a simple dialog after completion.  I can file a new bug about that after this gets in.

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: changed 1000 to 1024, cleaned up the last tab and the pref entity spacing

This patch should address all issues from Comment #19

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 341689 [details] [diff] [review]
changed 1000 to 1024, cleaned up the last tab and the pref entity spacing

Thx, looks good! r=mkmelin

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

Sweet!  Adding keywords and updating status whiteboard to nudge standard8 :)

Comment 24

[Mark Banner (:standard8)]

Checked in, changeset id: 498:b4fe0e98a33c