455069 - "Search Messages...", compact folders grayed out when IMAP folder is selected

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce:
1. Fetch SeaMonkey trunk build
2. Select a folder from an IMAP account, may also happen with POP3, dunno
2. Try to select Tools->Search Messages...

Results:
"Search Messages..." is grayed out and cannot be selected.

This is probably a regression, I think this might have to do with Bug 422814. It changed the function nsImapIncomingServer::GetPrefForServerAttribut under http://hg.mozilla.org/comm-central/annotate/e4f4569d451a/mailnews/imap/src/nsImapIncomingServer.cpp#l26. This function is called from nsImapIncomingServer::GetCanSearchMessages (http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#2465) which is called by http://hg.mozilla.org/comm-central/annotate/e4f4569d451a/mailnews/base/resources/content/mail3PaneWindowCommands.js#l865.
The old function code under nsImapIncomingServer::GetPrefForServerAttribut did not make that much sense (it did not use the prefSuffix param, see the patch/diff), but with the new code I get this bug here.

Comment 1

[Frank Wein [:mcsmurf]]

Confirmed with a TB build, so MailNews Core.

Comment 2

[[e-mail]]

I think the old code didn't touch the out variable if the pref didn't exist, and the callers all set it to true, but the new code sets it to false if the pref doesn't exist.

Comment 3

[Wayne Mery (:wsmwk)]

file|compact folders is toast too

Comment 4

[Frank Wein [:mcsmurf]]

I meant http://hg.mozilla.org/comm-central/annotate/e4f4569d451a/mailnews/imap/src/nsImapIncomingServer.cpp#l2691 in the first comment as first link.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Fix [Checkin: Comment 8]

What this patch does is to make GetPrefForServerAttribute act as it did before, i.e., don't change the value if the preference doesn't exist, unlike GetBoolValue.

Comment 7

[David :Bienvenu]

Comment on attachment 338413 [details] [diff] [review]
Fix
[Checkin: Comment 8]

ah, thx, Joshua.

Comment 8

[Joshua Cranmer [:jcranmer]]

Checked in as changeset b63a39af19cd.

Comment 9

[Frank Wein [:mcsmurf]]

Still broken for me with a SeaMonkey trunk build.

Comment 10

[Frank Wein [:mcsmurf]]

GetBoolValue always returns NS_OK, so the NS_FAILED check cannot work. Maybe a clean fix would be better (no longer use the prefValue arg as in/out param).

Comment 11

[Wayne Mery (:wsmwk)]

does this also break other things in some way?  message counts, filtering

Comment 12

[Wayne Mery (:wsmwk)]

can I kill comment 11? :)   ignore it - started with wrong profile.

Comment 13

[Frank Wein [:mcsmurf]]

Breaks basic functions of SeaMonkey MailNews (also see Bug 454936), so requesting blocking?.

Comment 14

[Magnus Melin [:mkmelin]]

Still broken with thunderbird also. The first time it might be non-grayed out, but it doesn't bring up the search dialog.

File | Compact and the Compact Button are always disabled for IMAP too.

Comment 15

[Frank Wein [:mcsmurf]]

Attachment: Idea for a patch

So, I had the idea to add a parameter with the default value to GetBoolValue. That parameter is assigned to the return value when no pref was found. Comments welcome.

Comment 16

[Frank Wein [:mcsmurf]]

Comment on attachment 338554 [details] [diff] [review]
Idea for a patch


>-  if (NS_FAILED(mPrefBranch->GetBoolPref(prefname, val)))
>-    mDefPrefBranch->GetBoolPref(prefname, val);
>+  if (NS_FAILED(mPrefBranch->GetBoolPref(aPrefname, aVal)))
>+    if (NS_FAILED(mDefPrefBranch->GetBoolPref(aPrefname, aVal)))
>+      *aVal = aDefaultValue;

Looks like I forgot { ... } after the first if-clause.

Comment 17

[Mark Banner (:standard8)]

(In reply to comment #15)
> Created an attachment (id=338554) [details]
> Idea for a patch
> 
> So, I had the idea to add a parameter with the default value to GetBoolValue.
> That parameter is assigned to the return value when no pref was found. Comments
> welcome.

This seems a bit like overkill, when we could just replicate the appropraite actions on the function. It also means we have an inconsistent set of APIs for getting the different types of prefs. Have you also checked for javascript usages? I guess making that default paramenter [optional] would save changing those as well.

Comment 18

[Wayne Mery (:wsmwk)]

painful to use latest trunk

Comment 19

[Frank Wein [:mcsmurf]]

Attachment: Another patch [Checkin: See comment 22]

This is basically the GetBoolValue function implemented in the GetPrefForServerAt function, just with the input parameter as default return value when getting the prefs failed.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 338632 [details] [diff] [review]
Another patch
[Checkin: See comment 22]

Please add a comment at the start of the function stating that the caller must initialise prefValue for the default value.

Note that we're accepting this in this format because those prefs should disappear because they are related to aol setups that weren't supported, and we believe they now are.

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #20)
> Note that we're accepting this in this format because those prefs should
> disappear because they are related to aol setups that weren't supported, and we
> believe they now are.

I filed bug 455326.

Comment 22

[Frank Wein [:mcsmurf]]

Pushed to c-c, changeset 713c34b1a232.

Comment 25

[Wayne Mery (:wsmwk)]

This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.