Match all messages in the filter and save search dialog hides UI elements

RESOLVED FIXED in Thunderbird 3

Status

Thunderbird
Mail Window Front End
--
minor
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Scott MacGregor, Assigned: Žiga Sancin)

Tracking

({polish})

Trunk
Thunderbird 3
polish

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
We shouldn't be hiding the filter rules when you choose Match all messages.

We should just disable the rules box.

Updated

11 years ago
OS: Windows XP → All
Hardware: PC → All

Updated

11 years ago
Severity: normal → minor
Keywords: polish
(Assignee)

Comment 1

11 years ago
Created attachment 264179 [details] [diff] [review]
disable listbox

I'm not sure if that's the right approach here, but it seems that there's no easier way to do it.
Assignee: mscott → bisi
Status: NEW → ASSIGNED
Attachment #264179 - Flags: review?(mscott)
(Reporter)

Comment 2

11 years ago
Comment on attachment 264179 [details] [diff] [review]
disable listbox

That turned out to be more complicated than I thought it would :)

Looks ok to me, asking Magnus for a peer review too.
Attachment #264179 - Flags: superreview+
Attachment #264179 - Flags: review?(mscott)
Attachment #264179 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 3

11 years ago
Thanks Scott. 

By the way, this patch also fixes the save search dialog, which uses the same xul overlay file.
Summary: Match all messages in the filter dialog hides UI elements → Match all messages in the filter and save search dialog hides UI elements

Comment 4

11 years ago
Comment on attachment 264179 [details] [diff] [review]
disable listbox

You should enable disabled elements again if the match all is deselected. Now you get stuck with a disabled input field when you toggle between match all of/any/all. 

I think JavaDoc syntax ( /** */ ) is preferable for function documentation, not that that file uses it much...
Attachment #264179 - Flags: review?(mkmelin+mozilla) → review-
(Assignee)

Comment 5

11 years ago
Good catch! Actually, the text input field gets enabled, but it's not focusable anymore. I'll look into that, thanks for the review.
(Assignee)

Comment 6

11 years ago
Created attachment 272325 [details] [diff] [review]
disable listbox, v2

Weird, just switching disabled attributes doesn't work for searchvalue textboxes. We also need to remove them.

I've also added java doc style comments and a tiny change to mailWidgets (I forgot to add xbl:inherits to the second xul:textbox element in the searchvalue binding).
Attachment #264179 - Attachment is obsolete: true
Attachment #272325 - Flags: review?(mkmelin+mozilla)

Comment 7

11 years ago
Comment on attachment 272325 [details] [diff] [review]
disable listbox, v2

>+ * @param matchAllValue
>+ *        boolean value from the first search term

Please put it on the same line.

>+      searchValueList[i].setAttribute("disabled", matchAllValue);
>+      if (!matchAllValue)
>+        searchValueList[i].removeAttribute("disabled");

Are both needed? I would have thought just removing the attribute was enough.

Looks quite good, but I noticed it doesn't disable the value dropdowns for Priority and Status.
(Assignee)

Comment 8

11 years ago
Comment on attachment 272325 [details] [diff] [review]
disable listbox, v2

Oops, I'll post a new patch.
Attachment #272325 - Attachment is obsolete: true
Attachment #272325 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 9

11 years ago
Created attachment 272343 [details] [diff] [review]
disable listbox, v2.1

I've added a few more xbl:inherits. :)

Regarding your question about using setAttribute and removeAttribute: I think in this case they are needed both, because a searchvalue element doesn't reset the value of the disabled attribute if only removeAttribute is used.
Attachment #272343 - Flags: review?(mkmelin+mozilla)

Comment 10

11 years ago
Comment on attachment 272343 [details] [diff] [review]
disable listbox, v2.1

Looks good now, thx!
r=mkmelin+mozilla
Attachment #272343 - Flags: review?(mkmelin+mozilla) → review+

Updated

11 years ago
Target Milestone: Thunderbird2.0 → Thunderbird 3
(Assignee)

Comment 11

11 years ago
Comment on attachment 272343 [details] [diff] [review]
disable listbox, v2.1

After numerous complications, I think this patch is ready to go. :) Checking for a sr again.
Attachment #272343 - Flags: superreview?(mscott)
(Reporter)

Comment 12

11 years ago
Comment on attachment 272343 [details] [diff] [review]
disable listbox, v2.1

yeah..bisi is back. looks good to me.
Attachment #272343 - Flags: superreview?(mscott) → superreview+
(Assignee)

Comment 13

11 years ago
I'd like to throw a new chicken into the tree. Yeah, this means you, attachment 272343 [details] [diff] [review]. :)
Keywords: checkin-needed

Comment 14

11 years ago
mozilla/mailnews/base/resources/content/mailWidgets.xml              1.118
mozilla/mailnews/base/search/resources/content/searchTermOverlay.js  1.49
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Version: 2.0 → Trunk
(Assignee)

Comment 15

11 years ago
Thanks Nickolay!

Comment 16

11 years ago
(In reply to comment #6)
>Weird, just switching disabled attributes doesn't work for searchvalue
>textboxes. We also need to remove them.
That's because they're really HTML <input> elments in disguise, and they disable themselves simply on the presence of a disabled attribute.

Updated

9 years ago
Depends on: 477254
You need to log in before you can comment on or make changes to this bug.