Closed Bug 451599 Opened 11 years ago Closed 11 years ago

Add preferences UI for disk cache size and clearing the cache

Categories

(Thunderbird :: Preferences, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: standard8, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Flags: wanted-thunderbird3+
Priority: -- → P2
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.
Depends on: 451620
Depends on: 451987
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.
(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. 
(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.
Whiteboard: [needs other pref removal]
Attached patch WIP for adding the pref (obsolete) — Splinter Review
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.
Assignee: nobody → clarkbw
Attached patch v2 of patch adds cache pref UI (obsolete) — Splinter Review
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.
Attachment #340649 - Attachment is obsolete: true
here's the screenshot of the change
updating flags and status whiteboard.  need to find reviewer
Assignee: clarkbw → nobody
Whiteboard: [needs other pref removal] → [needs review] [needs patch from bug 451620 checked-in]
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Look forward to testing this.
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.
Assignee: nobody → 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...
Attached patch v3 cleans up white space (obsolete) — Splinter Review
no changes to the screenshot needed, this just cleans up the white space problem

magnus: do you want to review this?
Attachment #340666 - Attachment is obsolete: true
Attachment #340948 - Flags: review?(mkmelin+mozilla)
We should calculate it correctly, using 1024. I filed bug 457906 to get the calculation fixed in firefox.
if the person who is committing this wants to make that change to the current patch it's fine with me.
(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.
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/
(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.
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 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.
Attachment #340948 - Flags: review?(mkmelin+mozilla) → review-
> >+    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.
This patch should address all issues from Comment #19
Attachment #340948 - Attachment is obsolete: true
Attachment #341689 - Flags: review?(mkmelin+mozilla)
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
Attachment #341689 - Flags: review?(mkmelin+mozilla) → review+
Sweet!  Adding keywords and updating status whiteboard to nudge standard8 :)
Assignee: clarkbw → nobody
Keywords: checkin-needed
Whiteboard: [needs review] [needs patch from bug 451620 checked-in]
Checked in, changeset id: 498:b4fe0e98a33c
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.