Closed Bug 473642 Opened 13 years ago Closed 12 years ago

the compact button should be disabled for saved searches

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: mkmelin, Assigned: mkmelin)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The compact button should be disabled for saved searches, as it does nothing for them.
Attached patch proposed fix (obsolete) — Splinter Review
This fixes it for saved searches, and also makes sure the compact button is disabled after startup (when a server node is selected) - I could reproduce that most of the times, but not always. Anyway, this is what the code should do as cmd_compactFolder is "compact all folders".
Attachment #361254 - Flags: review?(jminta)
Status: NEW → ASSIGNED
Summary: the compact button should be disabled for saved searches → the compact button should be disabled for saved searches (and on startup when account node selected)
Target Milestone: --- → Thunderbird 3.0b2
Comment on attachment 361254 [details] [diff] [review]
proposed fix

Gah...
Attachment #361254 - Attachment is obsolete: true
Attachment #361254 - Flags: review?(jminta)
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Maybe I can get it right this time:)
Attachment #361296 - Flags: review?(jminta)
jminta: r ping
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Comment on attachment 361296 [details] [diff] [review]
proposed fix, v2

This seems like a strange way to fix this. Right now isCommandEnabled* calls the selected folder's isCommandEnabled function. It seems to me that saved-search folders ought to return false there, which would cover this, no?

*Here I'm referring to the call to the terribly named function at the end of IsFolderCompactionEnabled, not the function-method of an nsICommandHandler.
The reason that's not enough is the startup case, where the defaultController will be used... and that needs to do all the same checks as the FolderPaneController.
Actually. I don't know what the startup thing was about anymore - i don't see it now.
Summary: the compact button should be disabled for saved searches (and on startup when account node selected) → the compact button should be disabled for saved searches
Attached patch proposed fix, v3Splinter Review
o button_compact is used both when the folder controller handles stuff and when the default handler does; and in the same way. Therefore i removed the handling from the folder controller, letting the default controller take all handling of it.

o function isCommandEnabled(cmd) was only used in very few places, so i'm removein that - didn't play nice with multi-select anyways.

o the compact was wrongly disabled for news - bug 460819.
Attachment #361296 - Attachment is obsolete: true
Attachment #377221 - Flags: review?(jminta)
Attachment #361296 - Flags: review?(jminta)
Comment on attachment 377221 [details] [diff] [review]
proposed fix, v3

+        return folders.length == 1 && folders[0].canRename &&
+               folders[0].isCommandEnabled("cmd_renameFolder");

+            (folder.server.type != "imap" || folder.server.canCompactFoldersOnServer) &&
+            folder.isCommandEnabled("button_compact");
I just had a long irc discussion with dmose about the absurdity of (1) splitting this logic between the front and back ends, with particular checks in each and (2) even worse, having a canRename attribute that doesn't tell you everything, but also requires you to check isCommandEnabled("cmd_renameFolder"). (The same applies for the compact stuff)

The tentative conclusion we came to was that nsIMsgFolder::isCommandEnabled should die. Whether its current logic should move to the front-end, or should move to the appropriate nsIMsgFolder attributes remains a trickier question. Please file a follow-up to remove the method and sort out the proper location for that logic.

r=jminta with that bug filed. And bonus points for removing functions from the global scope.
Attachment #377221 - Flags: review?(jminta) → review+
Filed bug 493016.

changeset:   2637:bb38d33f5744
http://hg.mozilla.org/comm-central/rev/bb38d33f5744

->FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 495873
You need to log in before you can comment on or make changes to this bug.