Closed Bug 549712 Opened 14 years ago Closed 10 years ago

Customize option in Search Messages is wonky

Categories

(Thunderbird :: Search, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: vlad, Assigned: aceman)

References

Details

Attachments

(1 file, 3 obsolete files)

wonky in two ways:

1) Selecting Customize... and clicking ok has Customize... remain as the selected header, which has no meaning.

2) Adding a header in the Customize... dialog box doesn't actually add it to the dropdown list until you add a new search row/delete the old one (or until you close the window and open a new one)
I can confirm 1) with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2pre) Gecko/20100314 Lanikai/3.1b2pre.

But 2) is WFM. Vlad What version are you using ? Can you try in safe-mode for 2 ?
(In reply to Ludovic Hirlimann [:Usul] from comment #1)
> I can confirm 1) 

usul, vlad, I don't quite get 1).
are you referring to the fact that the OK fails?


vlad...?

> But 2) is WFM. Vlad What version are you using ? Can you try in safe-mode
> for 2 ?
Severity: normal → minor
1) still exists. I think he means that after you add a new custom header and close teh Customize dialog (yes, 2 times) it is not preselected in the header dropdown menu. It still shows the item "Customize...".

It is the same in Filter edit dialog.
Version: unspecified → Trunk
Attached patch patch (obsolete) — Splinter Review
This is so glaringly bad it ruined testing my extension, and had to be fixed..
Assignee: nobody → alta88
Attachment #757992 - Flags: review?(mkmelin+mozilla)
This seems to be a relatively big patch for this bug. Why are you removing MissingCustomTerm functionality and the workaround for bug 212625? Is it no longer needed?
what?  it's removing lots of junk no longer needed, yes.
So can you explain why it is no longer needed? I see bug 212625 is closed now. What about the MissingCustomTerm functionality?
Comment on attachment 757992 [details] [diff] [review]
patch

Review of attachment 757992 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a good improvement. But so why is the missingterm stuff no longer needed?

::: mail/base/content/mailWidgets.xml
@@ +1536,5 @@
> +            {
> +              // Customize menuitem selected.
> +              let args = {};
> +              window.openDialog('chrome://messenger/content/CustomHeaders.xul',
> +                                "", 'modal,centerscreen,resizable,titlebar,chrome',

nit: prefer double quote where no reason to use single

@@ +1543,5 @@
> +              if (args.selVal)
> +                this.value = args.selVal;
> +            }
> +            else
> +              this.value = menulist.value;

if one if-else have braces, they all should have it.

::: mailnews/base/search/content/CustomHeaders.js
@@ +7,5 @@
>  
>  var gAddButton;
>  var gRemoveButton;
>  var gHeaderInputElement;
> +var gArgs;

since it's used just once, maybe juse window.arguments[0] directly there instead
Comment on attachment 757992 [details] [diff] [review]
patch

Review of attachment 757992 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review until the question about MissingCustomTerm is answered.

::: mail/base/content/mailWidgets.xml
@@ +1536,5 @@
> +            {
> +              // Customize menuitem selected.
> +              let args = {};
> +              window.openDialog('chrome://messenger/content/CustomHeaders.xul',
> +                                "", 'modal,centerscreen,resizable,titlebar,chrome',

nit: prefer double quote where no reason to use single

@@ +1543,5 @@
> +              if (args.selVal)
> +                this.value = args.selVal;
> +            }
> +            else
> +              this.value = menulist.value;

if one if-else have braces, they all should have it.

::: mailnews/base/search/content/CustomHeaders.js
@@ +7,5 @@
>  
>  var gAddButton;
>  var gRemoveButton;
>  var gHeaderInputElement;
> +var gArgs;

since it's used just once, maybe juse window.arguments[0] directly there instead
Attachment #757992 - Flags: review?(mkmelin+mozilla)
Attached patch nits (obsolete) — Splinter Review
it's no longer needed because it's no longer used, anywhere.  not sure what else can be added to that.  you can verify for yourself if unsure.
Attachment #757992 - Attachment is obsolete: true
Attachment #760235 - Flags: review?(mkmelin+mozilla)
Attachment #760235 - Flags: feedback?(kent)
(In reply to Magnus Melin from comment #11)
> ( That stuff was added in bug 495519 -
> http://hg.mozilla.org/comm-central/rev/db4d4379d404 )

the hack in that bug to make the customize menulist work (which fails anyway) is an embarrassment.  the evidence is before you.
Comment on attachment 760235 [details] [diff] [review]
nits

Review of attachment 760235 [details] [diff] [review]:
-----------------------------------------------------------------

Ok i think it's ok. But let's have rkent (as author of bug 495519 which added that part, also i think probably user in some extension) take a look before checking it in.
Attachment #760235 - Flags: review?(mkmelin+mozilla)
Attachment #760235 - Flags: review?(kent)
Attachment #760235 - Flags: review+
Attachment #760235 - Flags: feedback?(kent)
OS: Windows 7 → All
Hardware: x86 → All
"it's no longer needed because it's no longer used, anywhere."

If you are referring to the the MissingCustomTerm stuff, the issue here is what happens when a user defines a search using an addon with an nsIMsgSearchCustomTerm and then removes or disables that addon.

Why do you say that is not used anywhere?
Have you looked at the patch?  If a custom header has been removed, the menulist selection defaults to (attrib) value of 0 (Subject), same as a new search/filter.

The current code displays even more bustage that is this UI (aside from the menulist selection issues) when custom terms are removed.  No extension needed, simply add a custom string(s), create some saved search rules, then remove the custom header.  Sometimes the Missing string is in the search menulist but mostly it selects the prior header.

It's a followup bug to fix the keeping of a rules row whose header is removed; a row should not be built/should be removed if its header isn't there.  The current design is pointless and clunky, in addition to buggy.
(In reply to alta88 from comment #15)
> It's a followup bug to fix the keeping of a rules row whose header is
> removed; a row should not be built/should be removed if its header isn't
> there. 
That looks like a dataloss. You intend to remove the rule if the extension is available? I think the point of the original code was to preserve the rule, just mark it "missing" and keep it in the filter until the addon is enabled again so that the rule can start functioning.
1. It's not this bug.
2. No one said anything about removing a rule, but about not displaying an invalid row.
Comment on attachment 760235 [details] [diff] [review]
nits

The MissingTerm functionality of custom search terms was intentionally added to try to deal with the difficult problem of handling the case when a custom search term is made invalid, typically by disabling or removing an extension.

There are several problems with the removal of that functionality here.

First, the user has no warning about why a filter or search that used to work suddenly stops working. (Before this patch, the user saw both "Missing" in the search definition, as well as console error messages).

Second, you have replaced the search with a different term, but left it active in the editor. So for example, let me add a filter with a custom search term 'Folder Name Contains "test"' (using FiltaQuilla). If I then disable that extension, what the filter now shows with your patch is Subject Contains "test". So from the point of view of the user, some random search term has now replaced the original search term. But it is not really replaced, so in fact the search definition that is being shown is not what is actually in the message filter definition. If I then try to run that filter on a message that actually has "test" in the subject, it does not work, yet the filter definition looks fine.

I don't see either of these as improvements, but rather regressions.
Attachment #760235 - Flags: review?(kent) → review-
rkent, is the rest of the changes fine?
(In reply to :aceman from comment #19)
> rkent, is the rest of the changes fine?

Magnus already reviewed the patch, so I did not look at the rest of the patch, just the part that removed the stuff associated with custom search terms.
(In reply to Kent James (:rkent) from comment #18)
> Comment on attachment 760235 [details] [diff] [review]
> nits

> I don't see either of these as improvements, but rather regressions.

It's unfortunate you're wearing the wrong hat here.

That 'functionality' is the regression; it makes core Tb header Customize beyond broken in multiple visible user facing ways.  Did you happen to notice what happens when you 'select' Customize, leave it in the rule, close the window, then edit the filter again?

To prioritize a very edge usage case (and poorly functioning one at that), for a single extension with 5k users, at the expense of the core product, is irresponsible and rather unprecedented.
(In reply to alta88 from comment #21)
> 
> To prioritize a very edge usage case (and poorly functioning one at that),
> for a single extension with 5k users, at the expense of the core product, is
> irresponsible and rather unprecedented.

My comments have mostly been in response to "it's no longer needed because it's no longer used, anywhere.  not sure what else can be added to that.  you can verify for yourself if unsure."

So I have verified that in fact it IS used, shown where it is used, and how your patch breaks where it is used. Nowhere have I claimed that the custom search term feature should be prioritized over custom headers.

Are you claiming that it is impossible to fix this bug without also breaking the custom search term feature? So far you have not made that case, instead you have claimed that the code in question is "no longer needed", which is clearly not the case.
I am claiming the core Tb header Customize function does not need to, nor should, maintain a removed header rule with a Missing string, but should revert to a default existing header.  When a user removes a custom header, they intend to remove a custom header.  In all cases the dialog and backend should respect that user initiated action and clean up, also handling the case where a header was manually removed in the customHeaders pref string.

I am further claiming that fixing the existing bustage, for the extensionless core product, which obviates the need for the Missing string, _by far_ trumps your extension's needs and thus defines 'unused'.

(Tears are not shed for extensions due to core changes; the treeview properties array change will break hundreds).

Handling of removed custom headers in an extension disable/uninstall case is up to the extension, and there are simple methods to catch those events and process accordingly.  For example, in one of my extensions extra textfilters are added and end up stored in session.json; when uninstalled Tb throws on the remains.  So I clean up session.json.  I suggest you approach this bug from that perspective.
Comment on attachment 760235 [details] [diff] [review]
nits

At this point in time, I think that the best thing is to let Magnus, the original reviewer, make the call on this.

Let me though state my case more broadly.

First, I don't agree with "Tears are not shed for extensions due to core changes". I regularly search mxr.mozilla.org/addons to find how core changes will affect existing addons, and while maybe I don't shed tears, I certainly encourage changes to have a minimal impact on addons. Yes there are sometimes breaking changes needed, but you need to make a strong case for that, and I don't believe that has been made here yet.

The custom search term functionality is currently used in 6 addons on AMO, only one of which is mine. Because this functionality was specifically added to core to support addons, I feel like it is important for the core code to be as robust as possible in dealing with the effects of using that functionality. One issue is what happens to existing filters and searches that use custom terms, when the custom term is no longer defined. The best way to handle that, as was decided in the original bug to add that functionality, is to explicitly show the user when a missing custom search term is no longer present in a search expression. I don't see any way for an addon to accomplish the same thing after it is removed.

I encourage you to try to find some way to fix the current bug's issue without removing the missing custom term functionality from core.
(In reply to Kent James (:rkent) from comment #24)
> Comment on attachment 760235 [details] [diff] [review]
> nits
>
> I encourage you to try to find some way to fix the current bug's issue
> without removing the missing custom term functionality from core.

I do not believe there is any functionality worth keeping, the whole premise is not well thought out.  Consider the scenarios:

1. A user creates a custom header, creates some rules, then removes the header.  Clearly telling the user there is a missing header in the dialog is silly, in the 'yeah i know' category.  Any rules with that header in an open dialog revert to the default, and a case could be made for removing those rows (although there may be terms that the user would convert to another header).

2. An extension creates custom headers, and rules are created.  Later, the extension is uninstalled.  The extension
a) is polite enough to clean up the headers it added in customHeaders pref; in this case the dialog should _not_ show rows for those missing headers, and _should_ autoclean them from msgFilters.dat on filter dialog open otherwise it's just not smart code.  Keeping those remains is incorrect.  As I said, this might be a followup bug, specifics tbd.
b) is not polite and the pref string remains the same; not much changes in the rules dialog or behavior, I don't believe anything breaks (the backend should just ignore headers it can't handle), the user has to manually update the Custom headers list/rules if removal is desired.

3) An extension is disabled.  If the extension wants to keep rules in case it is reenabled, then it shouldn't clean up the customHeaders pref on a disable event!

There is no scenario whereby a confusing Missing item should be presented to the user, sorry.  It's not actionable, doesn't even show the terms for the rule, and needs to go away.
I can agree it's not optimal to have a "Missing" showing where it is now, and that should be communicated to the user in another way. What comment 18 describes still needs to be addressed though. 

We can't rely on extensions always doing the right thing, but even if we could, we can't trust users to remember to fix their filters before removing custom headers.
Magnus, I've already explained what should happen in a followup for a case where the header is no longer in customHeaders pref.  And no, nothing need be communicated to the user that is not actionable, how is that possibly useful and anything but confusing?  The code should handle it for them.  This Missing design likely wouldn't pass UX today.

The patch fixes gross multilevel UI failure for 20M users who have never touched a filter extension.  If you want to block that for a miniscule number of users, in an edge case, for a poorly designed and horribly implemented non-feature, it's your call.  I have nothing more to add (it all seemed obvious to me day one).
(In reply to alta88 from comment #17)
> 2. No one said anything about removing a rule, but about not displaying an
> invalid row.
From the later discussion it seems like you are changing an "invalid" term, not just hiding it...
(In reply to alta88 from comment #27)
> Magnus, I've already explained what should happen in a followup for a case
> where the header is no longer in customHeaders pref.  And no, nothing need
> be communicated to the user that is not actionable, how is that possibly
> useful and anything but confusing?  The code should handle it for them. 

But the new code makes it impossible to debug why the filter didn't work as the row with misssing would lie, and finding out why something happens is the first thing to do to achieve actionability. 

So is there a good reason you can't make the item show "missing" instead of defaulting to first? With that changed i think everyone would be happy.
(In reply to Magnus Melin from comment #29) 
> impossible to debug why the filter didn't work.

Are you actually saying that a filter, which required an extension to work, and which extension was then removed, so the filter doesn't work, needs debugging?  Is that a joke?

Someone else can pick this up.
Assignee: alta88 → nobody
(In reply to alta88 from comment #30)
> Are you actually saying that a filter, which required an extension to work,
> and which extension was then removed, so the filter doesn't work, needs
> debugging?  Is that a joke?

You notice the filter not working, then go to check what the criteria is. Yes i'd assume the UI gives me some clue the current filter is broken.
Whiteboard: [patchlove][has draft patch]
Attached patch alternate patch (obsolete) — Splinter Review
This is a stripped down patch that only fixes the immediate selection of the newly added or selected custom header.
Assignee: nobody → acelists
Attachment #760235 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8405799 - Flags: review?(mkmelin+mozilla)
Attachment #8405799 - Flags: review?(kent)
Comment on attachment 8405799 [details] [diff] [review]
alternate patch

Review of attachment 8405799 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this works for me (with the exception of the missing error messsage), so I am happy with this approach. If mkmelin or neil is also going to look at this, then I don't need to see it again. Either incorporate my suggested changes/fixes, or do whatever the next reviewer suggests.

::: mail/base/content/mailWidgets.xml
@@ +1576,5 @@
>                  return customTerm.name;
>                else
> +              { // The custom term may be missing after the extension that added it
> +                // was disabled or removed. We need to notify the user.
> +                Components.utils.reportError("Missing custom search term " + this.value);

Two issues here. First, this is changing from a message to an error. Is that intentional? If so, please explain why.

Second, this did not work for me. That is, I am not getting any error when a Missing Custom Term message appears, while without your patch I get a message.

The point of this message/error by the way is to communicate the "this.value" to the user, as that is a hint about what is missing.

So it seems to me to be just code churn here, and you would be better off to leave this unchanged.

::: mailnews/base/search/content/CustomHeaders.js
@@ +72,5 @@
>    {
>      Services.prefs.clearUserPref("mailnews.customHeaders"); //clear the pref, no custom headers
>    }
> +
> +  window.arguments[0].selectedVal = gHdrsList.selectedItem ? gHdrsList.selectedItem.label : null;

Although this appears to work without the "return true", a quick review of standard practice in ondialogaccept is that the "return true" is typically used. Either leave this, or explain why you are removing it.
Attachment #8405799 - Flags: review?(kent) → review+
(In reply to Kent James (:rkent) from comment #33)
> ::: mail/base/content/mailWidgets.xml
> @@ +1576,5 @@
> >                  return customTerm.name;
> >                else
> > +              { // The custom term may be missing after the extension that added it
> > +                // was disabled or removed. We need to notify the user.
> > +                Components.utils.reportError("Missing custom search term " + this.value);
> 
> Two issues here. First, this is changing from a message to an error. Is that
> intentional? If so, please explain why.
I assumed this actually produces an error (reading the "errorFlag"):
scriptError.init("Missing custom search term " + this.value,
                    null, null, 0, 0,
                    Components.interfaces.nsIScriptError.errorFlag,
                    "component javascript");
But you are right, it doesn't look like it is an error:
http://mxr.mozilla.org/comm-central/source/mozilla/js/xpconnect/idl/nsIScriptError.idl#18

> Second, this did not work for me. That is, I am not getting any error when a
> Missing Custom Term message appears, while without your patch I get a
> message.
I'll check that out.

> 
> The point of this message/error by the way is to communicate the
> "this.value" to the user, as that is a hint about what is missing.
Sure, I wanted to preserve that.

> So it seems to me to be just code churn here, and you would be better off to
> leave this unchanged.
I just found it strange to have this open-coded when at all other places we use Components.utils.reportError.
But if that doesn't work I put the code back.

> ::: mailnews/base/search/content/CustomHeaders.js
> @@ +72,5 @@
> >    {
> >      Services.prefs.clearUserPref("mailnews.customHeaders"); //clear the pref, no custom headers
> >    }
> > +
> > +  window.arguments[0].selectedVal = gHdrsList.selectedItem ? gHdrsList.selectedItem.label : null;
> 
> Although this appears to work without the "return true", a quick review of
> standard practice in ondialogaccept is that the "return true" is typically
> used. Either leave this, or explain why you are removing it.

Sure, no problem.
(In reply to Kent James (:rkent) from comment #33)
> Second, this did not work for me. That is, I am not getting any error when a
> Missing Custom Term message appears, while without your patch I get a
> message.
> 
> The point of this message/error by the way is to communicate the
> "this.value" to the user, as that is a hint about what is missing.
So I just checked it and I got the error message properly to the Error console.
I did: install Filtaquilla, create a filter with "Match regex" term. Disable Filtaquilla, restart TB, open the filter again. I got the message and disabled and "Missing custom term" label on the dropdown.
Flags: needinfo?(kent)
Comment on attachment 8405799 [details] [diff] [review]
alternate patch

Review of attachment 8405799 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with rkent's comments addressed r=mkmelin
Attachment #8405799 - Flags: review?(mkmelin+mozilla) → review+
aceman: not sure what to say about your comment 35. I was surprised myself that the error console message did not come up, so I retried it to confirm. Did you test it on a fresh profile? There are a few about:config settings that affect what errors show up.

I'll run a recompile here again to be sure though.
I just tried it in my testing profile and I have these prefs set:
user_pref("javascript.options.showInConsole", true);
user_pref("javascript.options.strict", true);
But I hope those do not affect messages marked as "error".
What that preference does is enable chrome errors to be shown in the console. Without it, they are not.

So that may be the issue here.
Flags: needinfo?(kent)
Setting both prefs to false still shows the message in the console.
Attachment #8405799 - Attachment is obsolete: true
Attachment #8410498 - Flags: review?(kent)
Comment on attachment 8410498 [details] [diff] [review]
alternate patch v2

Review of attachment 8410498 [details] [diff] [review]:
-----------------------------------------------------------------

I've looked at the patch, and confirmed that it fixes the issues I requested. Since it already has an r+ I'm not going to recompile and retest it.
Attachment #8410498 - Flags: review?(kent) → review+
Thanks.
Keywords: checkin-needed
Whiteboard: [patchlove][has draft patch]
https://hg.mozilla.org/comm-central/rev/b711c55bb174
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Depends on: 1018241
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: