Closed Bug 259914 Opened 21 years ago Closed 17 years ago

quicksearch toolbar no longer displays "recipient" when focused on "Sent"

Categories

(Thunderbird :: Mail Window Front End, defect, P4)

Product:
Thunderbird
Component:
Mail Window Front End
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Thunderbird 3.0b1

People

(Reporter: jayhenn, Assigned: lionel)

References

Details

Attachments

(1 file, 11 obsolete files)

Attachment #33734 without the emacs automated indentation tweaks
11.85 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10 Although it performs the way one would expect, Thunderbird .8 still displays "Subject or Sender" as opposed to "Subject or Recipient" when focused on a Sent mail folder. This may cause confusion if the user wasn't aware that they could actually type in recipient information. Previous versions of Thunderbird displayed the correct title. Reproducible: Always Steps to Reproduce: 1. Change context to a Sent-items folder 2. Examine quicksearch toolbar or dropdown 3.
xref bug 256545.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: quicksearch toolbar no longer diaplays "recipient" when focused on "Sent" → quicksearch toolbar no longer displays "recipient" when focused on "Sent"
Is there any chance that a "Recipient" search target could be added in all cases (being focused by upon by default in the context of a Sent folder), like Subject, Sender, Subject or Sender, and Message Body are now? It may be a useful to do in certain cases, and wouldn't add much clutter. Thanks -Jay
interestingly: 1. go to Sent folder 2. go to quicksearch field and select "Sender" or "Subject or Sender" 3. enter a string for a recepient actual results: looks like tbird still does the right thing and searches for recepients, even though the qs label says "sender."
(In reply to comment #3) > interestingly: > > 1. go to Sent folder > 2. go to quicksearch field and select "Sender" or "Subject or Sender" > 3. enter a string for a recepient > > actual results: looks like tbird still does the right thing and searches for > recepients, even though the qs label says "sender." yes, it does appear that way. So, Should this bug just change the text in the search prompt? Or, Shoud recipient be added to all searches? (which might help when trying to find which identity a message came into the inbox on)
I am a new user of thunderbird and I have to say this was very confusing. I really like the quick search by "sender" and I think it is potentially very useful, except that it also includes the receipient! I recommend the following list: Subject Sender Recipient Subject, Sender or Recipient Adam
I suggest the quicksearch toolbar text be changed from "Sender" to "To or cc" when looking at the Sent folder. As noted above, this is consistent with the way Thunderbird should (and does) respond.
I suggest the following options: - Subject - Sender - Recipient - Subject or Sender ==> default - Subject or Recipient ==> default only for Sent - ... (entire message, find in message, ...) "Recipient" may be marked as "To/cc:", although I would rather see "Recipient" label (and get results, that have the searched string under either TO or CC or BCC).
*** Bug 272444 has been marked as a duplicate of this bug. ***
putting the word "Recipient" in the searchbox only for Sent folder does not make sense. What if I have filed 'sent' messages in other folders? (like a date based fodler archive tree) Then it would not say Recipient even though I still want to search by Recipient. I think we should just replace 'Sender' -> 'Sender or Recipient' in each entry in the list. (or 'Sender, Recipient' as the context permits)
*** Bug 273630 has been marked as a duplicate of this bug. ***
I think recipient search should be available in all folders. In concur with the comment on the "qmail-like" experience. That's exactly how I am using tbird (with the help of the tempfromfcc extension), but tbird just isn't prepared to do that well. In Mulberry, for example, there's even a column called "Sender or Recipient", which I would also like for tbird...
*** Bug 270317 has been marked as a duplicate of this bug. ***
*** Bug 290401 has been marked as a duplicate of this bug. ***
Now that "To or CC:" is an explicit choice in the QS field-selector, this is a little more obviously broken. If you are in a Sent/Drafts folder, whether you select "Sender" or "To or CC" the match is against the recipient; not only is the "Sender" label still misleading, but the newer functionality of being able to match either field is unavailable in Sent/Drafts.
*** Bug 305225 has been marked as a duplicate of this bug. ***
*** Bug 314677 has been marked as a duplicate of this bug. ***
*** Bug 321464 has been marked as a duplicate of this bug. ***
would fix to this also fix bug 327600?
The bug is still present in TB version 2 beta 2 (20070116) and in version 3 alpha 1 (20070131). To help advance this bug it would be useful to separate two issues: The first issue is restricted to what prompted this bug: Thunderbird should be consistent with itself. If TB searches by recipient (as it does in the Sent folder) then it should display "Recipient" in the quick search window. This is simply making the UI coherent instead of confusing. The second issue is whether the search should be by Recipient only or by To/Cc, which is functionally different from the bug issue. If TB developers decide to make the Search itself more elaborate then this should obviously be reflected in the search window. It is important that this old (and somewhat trivial) first issue be resolved before TB 2 is released.
Blocks: 327600
(In reply to comment #19) > The second issue is whether the search should be by Recipient only or by > To/Cc, which is functionally different from the bug issue. Actually, no, that's not an issue -- "Recipient" is logically the same as "To or CC" in this case. There is a second issue, which is covered at bug 327600. > It is important that this old (and somewhat trivial) first issue be resolved > before TB 2 is released. Is it important enough that you're going to provide a patch?
Had I known how to patch TB, I would have done so a long time ago.
I'm happy to have a look at it. From a quick glance at the code, it doesn't look too hard. The one thing I need resolved first is exactly which functionality we want to end up with. I can think of three options: (A) The original proposal: Leave the functionality as it is and just change the menu names when recipient is going to be searched instead of sender. In which case, the "Sender" item will have to be disabled entirely in those folders, since it'll become a duplicate of "To or Cc". (B) Don't screw with the menu when changing to an outbound (Sent, Drafts etc.) folder, but instead add "Recipient" and "Subject or Recipient" as search options for ALL folders. I think this would actually be easier. The only con I can think of is that the user has to remember to switch their search option manually when going to that folder. C. Like (B), but when switching from an inbound to outbound folder, check if "sender" or "sender or subject" are selected and switch the selected option to the corresponding "recipient" one. Vice versa when switching back. The only con I can think of is that the user might intend for "sender" to be searched and not be aware that the search was switched silently. I favour (B) over (A), but am tempted to go for (C).
I vote for (B), but maybe with some user aid: if menu entries can have two font states (Bold/regular) i'd suggest Sender & Sender/subject in bold if the inbound folder is selected, and Recipient & Recipient/Subject bold if an outbound folder is selected, and regular font for all other entries. If not, then use Bullets to show the difference.
I haven't set review because the concept is pending feedback. This patch is just option (B). It adds "Subject, To or Cc" as a permanent option to the quicksearch menu, and removes the mailbox-dependent morph in function of the "Subject or Sender" menu option. But is this what we want? I'm tempted to also switch menu options when the nature of the mailbox changes, as per option (C).
I voted for B because of the silent action If a redesign is undertaken, then I would prefer a basic menu for Input & Sent and a complete menu list for other folders. Input: Sender & Subject (default) Sender Subject Other... -> Sent: Recipients & Subject (default) Recipients Subject Other... -> Another point : I do not see why the menu is not coherent : it says "Sender" (and not "From") and then "To or Cc" and not "Recipients" (note the plural).
(In reply to comment #24) > This patch [...] removes the mailbox-dependent morph in > function of the "Subject or Sender" menu option. > > But is this what we want? I'm tempted to also switch menu options when the > nature of the mailbox changes, as per option (C). I suspect the reason the criteria change with the nature of the folder is that the default address columns displayed are Sender (only) for incoming folders and Recipient (only) for outgoing. That's an older behavior carried over from when the quicksearch field *only* searched 'Subject or Sender'. Personally, I like the behavior. Since the quicksearch field always clears when you switch a folder, and so the cue text is always displayed when you see a new folder, I think changing the menu with the folder is OK for newusers -- it won't be a source of confusion -- so long as the cue text is accurate. Also: if this behavior is maintained, then the additional "Subject or Recipient" selection (for Inboxes) isn't really necessary. "Subject or Sender" was intended to just have a field that you didn't have to switch depending on which you were searching for, to make it quicker to use, not because people are often looking for messages where the string is appearing in either field. (In reply to comment #25) > If a redesign is undertaken, then I would prefer a basic menu for Input & > Sent and a complete menu list for other folders. Having a third state for non-special folders is a good idea, worth following up on, but (I'd say) beyond the scope of this bug.
(In reply to comment #26) > (In reply to comment #24) > > Personally, I like the behavior. Since the quicksearch field always clears > when you switch a folder, and so the cue text is always displayed when you see > a new folder, I think changing the menu with the folder is OK for newusers -- > it won't be a source of confusion -- so long as the cue text is accurate. > > I wonder if your assertion is directed at the present situation or is it directed to a future situation? I have Sender & Subject columns for Input folder and Recipient & Subject columns for Sent folder - which makes sense to me - but the cue text remains at whatever it was set to when I change folders, instead of adapting to the context, which is how it should be. An additional note: If I create a new folder, say 2006, and then create in it folders Input and Sent, TB does not recognize them as Input and Sent and I cannot have different columns displayed - both are either Sender or Recipient (or both). Menu switching will not work on these folders.
(In reply to comment #27) > (In reply to comment #26) > the cue text remains at > whatever it was set to when I change folders, instead of adapting to the > context, which is how it should be. I know. That's what this bug is about. > An additional note: If I create a new folder, say 2006, and then create in it > folders Input and Sent, TB does not recognize them as Input and Sent and I > cannot have different columns displayed - both are either Sender or Recipient > (or both). Menu switching will not work on these folders. Right. Only "special" Draft/Template/Sent folders (and their subfolders) are treated as "outboxes" for this purpose. xref bug 169025.
This new patch addresses option (C) - it checks for switches between "sent"-type folders (Sent/Drafts/Templates/Outbox) and "received"-type folders (the rest). When switching from a received to a sent folder and the search type contains "Sender", then it'll switch to the equivalent search type containing Recipient. And vice versa if the switch is the other way. Everything works fine except for one small nit : the popup menu with the search terms fails to update. So even though the search might now be "To or CC", and the faint text in the search box reflects that, the checked menu item would still appear to be "Sender". If you quit and restart, it notices the change and draws the menu properly, but other than that, the only way to update is to manually select the proper item. Things like |quickSearchMenu.selectedItem = newMenuItem;| don't work. I've tried a number of combinations of that. Any ideas? In practice, the search change executes correctly, so this is only an issue of appearance.
Attachment #255325 - Attachment is obsolete: true
I found the problem. The menu now checks the correct item. This patch seems to be complete, and implements option (C) from comment 22. I've kept the cumbersome "To or CC" terminology, rather than "Recipient", because I don't want to start a vocab argument and make this any messier than it has to be ;) I won't request review just yet, because I'd like feedback (can you break it?) and it'd also like to investigate something like Raanan mentioned in comment 25. But if I find it's all too hard for now, at least there's now something functional that could go into Tb 2.0. With respect to comment 25, ideally, I'd like the submenu to be "Custom" and contain checkbox items, so you could select whichever combination search terms you desired. Some aspects of that aren't too hard to implement, but I'm still wary of the logistics of embedding a "checkbox" submenu inside of a "radio" menu. I guess I'll see...
Attachment #255525 - Attachment is obsolete: true
(In reply to comment #30) > I found the problem. The menu now checks the correct item. Actually, I should have said that Enn on XulPlanet forums found the problem ;) Whoever "db48x" is on #developers was extremely helpful, too.
That works very well. If you are looking at an Inbox and select "To or CC" as the criterion, and then switch to Sent, the criterion remains "To or CC"; then when you switch back to the Inbox, the criterion changes to "Sender". Minor, but other criterion changes persist across folder changes. The one thing I would do differently (and this is obviously a minor matter about which reasonable people can disagree) is: in a 'special' folder, I would hide the "Subject or Sender" menu item, and in a Inbox/non-special folder, I would hide the "Subject, To or CC" item -- for the reason in comment 26 (my third paragraph). But this would require changing the behavior noted above. Is the 'autocheck' attribute necessary on the Save Search menu item?
(In reply to comment #32) > If you are looking at an Inbox and select "To or CC" as the criterion, and then > switch to Sent, the criterion remains "To or CC"; then when you switch back to > the Inbox, the criterion changes to "Sender". Minor, but other criterion > changes persist across folder changes. Yeah, it's hard to do anything about this, unless I save a search criterion option for each folder, and only choose the default search when no option is already stored. The downside to that would be that the search criteria may switch any time folders are changed, and from "neutral" criteria like "subject", too. It may end up being more confusing. What ya think? > The one thing I would do differently (and this is obviously a minor matter > about which reasonable people can disagree) is: in a 'special' folder, I would > hide the "Subject or Sender" menu item, and in a Inbox/non-special folder, I > would hide the "Subject, To or CC" item -- for the reason in comment 26 (my > third paragraph). But this would require changing the behavior noted above. That would probably make sense, for the sake of a simple UI. It's probably worth changing the order of the subject/recipient items in the menu, depending on window type, too. Unless the change in expected order might confuse the user more. > Is the 'autocheck' attribute necessary on the Save Search menu item? The inclusion of that was an accident. It'll be gone in the next patch. BTW, after some thought, I don't want to go with the "custom" search menu idea. To be done right, it really would need a custom configuration window, not in a menu. So I'll leave it for a possible future addition.
(In reply to comment #33) > (In reply to comment #32) > > If you are looking at an Inbox and select "To or CC" as the criterion, and > > then switch to Sent, the criterion remains "To or CC"; then when you switch > > back to the Inbox, the criterion changes to "Sender". Minor, but other > > criterion changes persist across folder changes. > > Yeah, it's hard to do anything about this, unless I save a search criterion > option for each folder, and only choose the default search when no option is > already stored. Alternately: if "To or CC" is selected while viewing Inbox, and you switch to Sent, then change to "Sender", and vice versa. That is, do the same thing for the non-default menu selection as you would for the default. > It's probably worth changing the order of the subject/recipient items in the > menu, depending on [folder] type, too. Unless the change in expected order > might confuse the user more. I was thinking about that. The possibilities are: Inbox Sent (option 1) Sent (option 2) ----------------- ------------------- ------------------- Subject Subject Subject Sender Sender To or CC Subject or Sender Subject or To or CC Subject or To or CC To or CC To or CC Sender Entire Message Entire Message Entire Message Option 1 keeps the actual items in the same location, but substitutes a different item for the Subject+ item. Option 2 keeps the logical items in the same location -- it swaps "Sender" and "To or CC", which is a reflection of what's changed in the first place. > BTW, after some thought, I don't want to go with the "custom" search menu > idea. To be done right, it really would need a custom configuration window, > not in a menu. I'm not sure what you mean by that.
1. The "Subject or *" option that is least useful is now hidden for the appropriate folder. 2. Menu items are reordered when the folder type changes, as per option 2 of Mike's suggestions in comment 34. The only deviation from his suggestion is that I placed "Entire Message" option before the least useful "sender" / "to or cc" option, since it's more important than the latter. But I'm open to other options. 3. In theory, I shouldn't need to manually uncheck the old menu selection before checking the new one, but for some reason Tbird is resistant to that in many circumstances, and leaves multiple selections checked, which leads to confusion when switching folders. I spent hours trying to work out why and still can't, so I'm going with the uncheck option. Any more feedback? :) If not, I'll tag it for review. Oh, and: > > BTW, after some thought, I don't want to go with the "custom" search menu > > idea. To be done right, it really would need a custom configuration window, > > not in a menu. > I'm not sure what you mean by that. It was a reference to the end of comment 30, but it's not important, as I'm not going with that.
Attachment #255677 - Attachment is obsolete: true
For those who have combined sent/inbox I'm not sure if this is finished. For example I'm not sure I agree with all of comment 32. Can you screen shot or list the choices one would see for inbox? And FWIW I would like to see a to+cc+from (combo) item - but perhaps that's different bug.
The list of menu options, in order, for inbox and sent are (with the current patch): Inbox Sent ----------------- ----------------- Subject Subject Sender To or CC Subject or Sender Subject, To or CC Entire Message Entire Message To or CC Sender I could add the other "Subject or *" options to the end of the menu lists, rather than hiding them, if you really think people will use them. > And FWIW I would like to see a to+cc+from (combo) item - but perhaps that's > different bug. Do you think many people would use that? The number of options will make the menu hard to navigate if we add too many, but again, it's easy to add if you really think it's useful. But it might be something that would be better in a "Custom Search" submenu, which I alluded to at the end of comment 30, and which is bigger than this bug.
Exactly! I personnally always patch my Thunderbird to have a "Header" option which searcher all fields in the header. Usually I don't care in which field the search term appears, and it's a waste of time to have to select this manually. Heck, Google Desktop manages to find most of what you need without ANY search options! I find it quite frustrating that Thunderbird is so stuck with its initial feature set. What would be more natural than to give the user the ability to define the search scopes himself?
Comment on attachment 256256 [details] [diff] [review] Patch v4 - menu item hiding and reordering added Scott, when you have time, could I please get a review of this (trunk)? :) As commenters have said, there is probably merit in adding more search options, possibly in a customize menu, but I would like this to be reviewed as it is. It fixes the problem, and removes no functionality that already existed. Any further enhancements may require some heavy mods to the code (I've already been experimenting), and I'll open a new bug for that.
Attachment #256256 - Flags: review?(mscott)
(In reply to comment #37) > > And FWIW I would like to see a to+cc+from (combo) item - but perhaps that's > > different bug. > > Do you think many people would use that? I don't know of any way to tell. But there is a class of users that would - for example those who do a lot of searches, and those who want their search not sorted by user. One of my heaviest used advanced searches is to find messages that were sent and received from one person or a domain. It is impossible to do this quickly with advanced search. >The number of options will make the > menu hard to navigate if we add too many, but again, it's easy to add if you > really think it's useful. But it might be something that would be better in a > "Custom Search" submenu, which I alluded to at the end of comment 30, and which > is bigger than this bug. the current number is 5. I don't thing 6 or 7 would be unreasonable.
The advanced search in Edit->Find->Search Messages could do this, though: (1) it probably should be linked from the search bar, so people know it exists, and (2) it doesn't remember your last search, which would suck if you wanted to use a specific custom search regularly Anyway, whatever improvements come next really need to be kept for another bug.
Requesting blocking Tb 2 (though the current patch is trunk, it should be easy to apply to both). Also note bug 372068, which I added for further improvement ideas.
Flags: blocking-thunderbird2?
we're passed the string freeze for tb2.
Flags: blocking-thunderbird2? → blocking-thunderbird2-
(In reply to comment #43) > we're passed the string freeze for tb2. Sorry Scott :( I should have noticed that. The only bit that pertains to l10n is the addition of "Subject, To or CC". Admittedly that's an important part of the change, but in the interests of at least a partial fix for Tb 2, would you still accept a patch that just fixed menu order and the functionality of "Sender" in outgoing mailboxes? At least one of two menu items would then be worded correctly. Maybe leave the incorrectly-named "Subject or Sender" still doing the same job as "Subject, To or CC"?
QA Contact: front-end
Assignee: mscott → nobody
this is still a problem with 2.0.0.14. I agree with Wayne thats just a string correction from "Subject or Sender" to "Subject, Recipient and CC:".
Could somebody from the original poster please check whether functionality is preserved by the adapted patch?
Attachment #330758 - Flags: review?(dmose)
(In reply to comment #46) > Created an attachment (id=330758) > Could somebody from the original poster please check whether functionality is > preserved by the adapted patch? From reading the patch, I think the following hunk: @@ -461,24 +462,24 @@ function createSearchTerms() // create, fill, and append the from (or recipient) term if (gSearchInput.searchMode == kQuickSearchFrom || gSearchInput.searchMode == kQuickSearchFromOrSubject) { term = gSearchSession.createTerm(); value = term.value; value.str = termList[i]; term.value = value; - term.attrib = searchAttrib; + term.attrib = nsMsgSearchAttrib.Subject; term.op = nsMsgSearchOp.Contains; term.booleanAnd = false; searchTermsArray.AppendElement(term); } Should be changed to: @@ -461,24 +462,24 @@ function createSearchTerms() // create, fill, and append the from (or recipient) term if (gSearchInput.searchMode == kQuickSearchFrom || gSearchInput.searchMode == kQuickSearchFromOrSubject) { term = gSearchSession.createTerm(); value = term.value; value.str = termList[i]; term.value = value; - term.attrib = searchAttrib; + term.attrib = nsMsgSearchAttrib.Sender; term.op = nsMsgSearchOp.Contains; term.booleanAnd = false; searchTermsArray.AppendElement(term); }
Attached patch Fixed typo in previous patch (obsolete) — — Splinter Review
Fixed a typo in the previous patch (searchRrecipient -> searchRecipient)
Attachment #330758 - Attachment is obsolete: true
Attachment #331621 - Flags: review?(dmose)
Attachment #330758 - Flags: review?(dmose)
Error inherited from previous previous patch: It would search only on subject in a FromOrSubject search, and only on subject on a From search...
Attachment #331623 - Flags: review?(dmose)
Attachment #331623 - Attachment is obsolete: true
Attachment #331628 - Flags: review?(dmose)
Attachment #331623 - Flags: review?(dmose)
next step is review?
Attachment #256256 - Attachment is obsolete: true
Attachment #256256 - Flags: review?(mscott)
Attachment #331621 - Flags: review?(dmose)
Attachment #331621 - Attachment is obsolete: true
Comment on attachment 331628 [details] [diff] [review] port patch #331623 to CVS trunk as of 2008/7/29 I'll steel the review for this... Please try to keep the lines @ 80 chars when it doesn't hurt the overall look of the code, there is a few places you should fix this. For some reason, the menu separator gets put in the wrong place (after the two first items). Maybe it needs an ordinal... Especially as the default ordinal is 1. --- >+ if (IsSpecialFolder(gMsgFolderSelected, MSG_FOLDER_FLAG_SENTMAIL | MSG_FOLDER_FLAG_DRAFTS | >+ MSG_FOLDER_FLAG_QUEUE, true)) I think MSG_FOLDER_FLAG_TEMPLATES should be included as well. > <menupopup id="quick-search-menupopup" > value="2" >- persist="value" Is this necessary? > <menuseparator id="afterSearchMessageBodySeparator"/> Probably wanna change this to beforeQuickSearchSaveAsVirtualFolder... > <menuitem id="quickSearchSaveAsVirtualFolder" >- value="5" >+ value="99" >+ ordinal="99" > label="&saveAsVirtualFolderMenu.label;" > oncommand="saveViewAsVirtualFolder()"/> I might be wrong, but i don't see why this would need a value at all... >+ >+// If switching from an "incoming" (Inbox, etc.) type of mail folder, to an "outbound" (Sent, Drafts etc.) >+// type, and the current search type contains 'Sender', then switch it to the equivalent 'Recipient' Doxygen/javadoc /** */ style comments for the function documentation please (and limit to 80 chars).
Attachment #331628 - Flags: review?(dmose) → review-
Blocks: 387505
Wayne Woods: are you planning to make an updated patch, or?
Magnus: someone else (Lionel Mamane?) is developing this patch now.
Ok, I just wanted to make sure someone was on it:) Lionel: assigning to you then!
Assignee: nobody → lionel
Flags: wanted-thunderbird3+
Priority: -- → P4
Target Milestone: --- → Thunderbird 3.0b2
(In reply to comment #55) > Ok, I just wanted to make sure someone was on it:) > Lionel: assigning to you then! Err... I just corrected an error I found in a previous patch; I wasn't aware that meant taking responsibility for the whole patch :) But if that's the way to getting this bug fixed, I'm willing to have a go at it. If I'm supposed to submit a patch I stand fully 100% behind, then the following changes: - don't hide quicksearch choices, only change the default depending on folder type; I explain in comment 8 of bug 387505 why I think the choice should stay available. - don't muck with order in the menu, it is too confusing (invisibly modal instead of mode-free) for CTRL+ARROW_UP/CTRL+ARROW_DOWN quicksearch type selection; again, just change the default (In reply to comment #52) > (From update of attachment 331628 [details] [diff] [review]) > I'll steel the review for this... Thanks. > Please try to keep the lines @ 80 chars when it doesn't hurt the overall look > of the code, there is a few places you should fix this. Fixed, but only for the lines actually added by the patch; I presume doing a general janitorial pass at the changed files is not appropriate. The files have an Emacs header asking for a 4-space indentation step, but they seem to mix two-space steps and four-space steps quite liberally, so I'm confused as to what indentation step I should use. Please advise. > For some reason, the menu separator gets put in the wrong place (after the two > first items). Maybe it needs an ordinal... Especially as the default ordinal is > 1. Fixed. >>+ if (IsSpecialFolder(gMsgFolderSelected, MSG_FOLDER_FLAG_SENTMAIL | MSG_FOLDER_FLAG_DRAFTS | >>+ MSG_FOLDER_FLAG_QUEUE, true)) > I think MSG_FOLDER_FLAG_TEMPLATES should be included as well. OK >> <menupopup id="quick-search-menupopup" >> value="2" >>- persist="value" > Is this necessary? No idea; don't know what its effect it. Removed it (removed the removal, thus readded the line). >> <menuseparator id="afterSearchMessageBodySeparator"/> > Probably wanna change this to beforeQuickSearchSaveAsVirtualFolder... I changed it to quickSearchAfterSearchOptions, as that's what I'm using it as in my changes to search.xml. That id does not seem to be referenced in the rest of the codebase, but I'm a bit afraid it is referenced in an extension? >> <menuitem id="quickSearchSaveAsVirtualFolder" >>- value="5" >>+ value="99" >>+ ordinal="99" >> label="&saveAsVirtualFolderMenu.label;" >> oncommand="saveViewAsVirtualFolder()"/> > I might be wrong, but i don't see why this would need a value at all... Indeed, but investigating whether it did sent me in the direction of a needed update to search.xml; I fixed and enhanced the code there (the code was making assumptions broken by the previous patch; I also removed the need to manually keep magical constants in mailWindowOverlay.xul and search.xml synchronised: search.xml now looks up the value dynamically. >>+// If switching from an "incoming" (Inbox, etc.) type of mail folder, to an "outbound" (Sent, Drafts etc.) >>+// type, and the current search type contains 'Sender', then switch it to the equivalent 'Recipient' > Doxygen/javadoc /** */ style comments for the function documentation please > (and limit to 80 chars). OK
Attached patch updated patch, see comment 56 (obsolete) — — Splinter Review
Attachment #331628 - Attachment is obsolete: true
Attachment #336556 - Flags: review?(mkmelin+mozilla)
(In reply to comment #56) > > Please try to keep the lines @ 80 chars when it doesn't hurt the overall look > > of the code, there is a few places you should fix this. > > Fixed, but only for the lines actually added by the patch; I presume doing a > general janitorial pass at the changed files is not appropriate. Yes, only fix it at places you touch the code anyway. (so skip the change you did around viewDebug) > The files have an Emacs header asking for a 4-space indentation step, but they > seem to mix two-space steps and four-space steps quite liberally, so I'm > confused as to what indentation step I should use. Please advise. We try to use 2-space indentation for "new" code, so where it's inconsistent in the file, prefer 2-space.
Comment on attachment 336556 [details] [diff] [review] updated patch, see comment 56 Seems to work fine, just a few minor things: 2 space indention here please >+ const outFolderFlagMask = MSG_FOLDER_FLAG_SENTMAIL | >+ MSG_FOLDER_FLAG_DRAFTS | MSG_FOLDER_FLAG_QUEUE | >+ MSG_FOLDER_FLAG_TEMPLATES; >+ if (IsSpecialFolder(gMsgFolderSelected, outFolderFlagMask, true)) >+ { >+ if (!gPrevSelectedFolder || >+ !IsSpecialFolder(gPrevSelectedFolder, outFolderFlagMask, >+ true)) >+ onSearchFolderTypeChanged(1); >+ } >+ else >+ { >+ if (!gPrevSelectedFolder || >+ IsSpecialFolder(gPrevSelectedFolder, outFolderFlagMask, >+ true)) >+ onSearchFolderTypeChanged(0); >+ } >+ >+ <menuseparator id="quickSearchAfterSearchOptions" >+ ordinal="6"/> Align ordinal with id. >+ const nextMenuItem = menuPopup.getElementsByAttribute('ordinal', --menuPopupOrdinal)[0]; Here, and in the similar places I suppose const is technically correct, but it's usually not used for non constants (like numbers, strings), so change it to "var" or "let". >+ * @param switchedInToOut >+ * whether one switched from an "incoming" to an "outbound" folder >+ * (1) or vice-versa (0) >+ */ Why not just call switchedInToOut isInOutboundFolder and true/false >+function onSearchFolderTypeChanged(switchedInToOut)
Attached patch patch updated to comment #59 (obsolete) — — Splinter Review
I also changed the Emacs mode-lines so that the automatic Emacs indentation gives results in the same style as the rest of the file, that is if () { foo } instead of if () { foo } (In reply to comment #59) >>+ * @param switchedInToOut >>+ * whether one switched from an "incoming" to an "outbound" folder >>+ * (1) or vice-versa (0) >>+ */ > Why not just call switchedInToOut isInOutboundFolder and true/false OK; I called it "isOutboundFolder" so that the name makes it clear what "true" means. >> +function onSearchFolderTypeChanged(switchedInToOut)
Attachment #336556 - Attachment is obsolete: true
Attachment #337340 - Flags: review?(mkmelin+mozilla)
Attachment #336556 - Flags: review?(mkmelin+mozilla)
Just in case you did _not_ want the emacs automation related tweaks
Attached patch patch updated to comment #59 (obsolete) — — Splinter Review
Whitespace damage hunk slipped by, sorry
Attachment #337340 - Attachment is obsolete: true
Attachment #337340 - Flags: review?(mkmelin+mozilla)
Attachment #337342 - Attachment description: Attachment #33734 without the → Attachment #33734 without the emacs automated indentation tweaks
Attachment #337344 - Flags: review?(mkmelin+mozilla)
Attachment #337344 - Attachment is obsolete: true
Attachment #337344 - Flags: review?(mkmelin+mozilla)
Comment on attachment 337342 [details] [diff] [review] Attachment #33734 [details] without the emacs automated indentation tweaks Looks good.
Attachment #337342 - Flags: review+
Before checking in i tweaked two line wrappings, removed the mode-line and changed the name of the separator to quickSearchAfterLastOptionSeparator. changeset: 285:2b3c08799c06 http://hg.mozilla.org/comm-central/rev/2b3c08799c06 ->FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Version: 0.9 → Trunk
Depends on: 454808
Nit: on testing with TB3.blahblah-0914, I'm seeing the cue text not getting displayed when the focus is elsewhere. STR: 0) set the criterion in Inbox to "Subject or From". Exit program. Restart. 1) Select Inbox, examine the cue text. 2) Select Sent, examine the cue text. 3) Change criterion to "To or CC." 4) Shift focus elsewhere, examine the cue text. 5) Select Inbox, examine the cue text. Actual results: (1) and (2) show the correct "Subject or From" and "Subject or To or CC" (3) and (4), the field is blank Expected results: (3) and (4) show the cue text correctly as "To or CC" and "From", respectively. This can be reset by re-selecting the "Subject or..." criterion, or by simply selecting "Subject." I haven't done enough testing to tell whether the problem is specific to the From/To-or-CC cases only.
The cue not being displayed at all is another bug, #453368.
Verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080928032854 Shredder/3.0b1pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080926031646 Shredder/3.0b1pre ID:20080926031646
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: