457745 - Filters: Auto-assign a meaningful name.

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect, P3)

Description

[Matt Dudziak]

Instead of 'Untitled Filter xx', filters should be assigned a meaningful name automatically, based upon the search terms.

Comment 1

[Matt Dudziak]

Attachment: Patch file

Patch file that adds a function to assign names to filters based upon the first search/match term.

Comment 2

[[e-mail]]

Comment on attachment 340975 [details] [diff] [review]
Patch file

>   saveSearchTerms(gFilter.searchTerms, gFilter);
> 
>+  AssignMeaningfulName();
I don't think this is the right place to do this. Problems:
1. If you blank out the name, then you still get an error that you could have fixed by assinging a meaningful name
2. You'll rename existing untitled filters
3. You could create duplicate names for similar filters
I think that you should start with a blank name, and if the user doesn't enter one (or blanks the name of an existing filter) then create a meaningful name and uniquify it if necessary. Only if we fail to assign a meaningful name should we then alert.

>+    // The text on popup menus is easy to access (activeItem.label),
>+    // but for whatever reason, there is no single place to get the string
>+    // value of the edit field.
Does activeItem.value not work?

Comment 3

[Matt Dudziak]

(In reply to comment #2)
> (From update of attachment 340975 [details] [diff] [review])
> >   saveSearchTerms(gFilter.searchTerms, gFilter);
> > 
> >+  AssignMeaningfulName();
> I don't think this is the right place to do this. Problems:
> 1. If you blank out the name, then you still get an error that you could have
> fixed by assinging a meaningful name

I see your point. I could easily change the check for default name to include a blank name. Would then need to remove the check for blank names (~6th line of function saveFilter() ).

> 2. You'll rename existing untitled filters

This code does not rename existing untitled filters. In my test setup I have multiple filters "Untitled Filter 1", "Untitled Filter 5", "Untitled Filter 12", etc. Those filters are left as they are. If you try to rename a filter to "Untitled Filter" (why you would do that, I have no idea), then yes, this code will attempt to rename that filter.

> 3. You could create duplicate names for similar filters

Is this a problem? I have tested with 2 filters with the same name and the only real problem I have experienced is that if I try to edit one of those filters I am warned about a duplicate name upon save (if I make a change). That is unpleasant and I'll see if I can work around that, but I am curious if there is a reason that same-named filters are not permitted. I've seen no ill-effects.

> I think that you should start with a blank name, and if the user doesn't enter
> one (or blanks the name of an existing filter) then create a meaningful name
> and uniquify it if necessary. Only if we fail to assign a meaningful name
> should we then alert.

OK, I'll look at that....

> 
> >+    // The text on popup menus is easy to access (activeItem.label),
> >+    // but for whatever reason, there is no single place to get the string
> >+    // value of the edit field.
> Does activeItem.value not work?

Yes, activeItem.value works perfectly. Thank you! I'd swear I looked at that (or ATTEMPTED to look at it) in Venkman and it was undefined. I don't have the best of luck using Venkman. I have simplified the code to use that. 


Thanks much for the input! I'll look a this over the next few days. I would appreciate your input on my question above regarding the reason for duplicate names being bad.

Thanks much,
Matt

Comment 4

[David :Bienvenu]

Filter names are a convenience for the user. They show up in the filter list for editing, and in the filter log.  But the filter running code doesn't care at all about the filter names. That being said, it's confusing for the user if they have a couple filters with the same name.

Comment 5

[Matt Dudziak]

(In reply to comment #4)
> Filter names are a convenience for the user. They show up in the filter list
> for editing, and in the filter log.  But the filter running code doesn't care
> at all about the filter names. That being said, it's confusing for the user if
> they have a couple filters with the same name.

Good to know.

While I agree that a couple of filters with the same name is a bit confusing, I think it is much better to have multiple "Subject contains 'Foo'" filters than to have multiple "Untitled Filter xx" filters. At least the former is somewhat descriptive.

Thanks for the info.
Matt

Comment 6

[David :Bienvenu]

Comment on attachment 340975 [details] [diff] [review]
Patch file

clearing review request based on Neil's -. Awaiting new patch. Thx for working on this.

Comment 9

[Matt Dudziak]

Attachment: Updated patch

Updated patch file.
Behavior is as follows:
New filter, user assigns name. User-assigned name is used (warned if a duplicate name).
New filter, user does NOT assign name. Name auto-assigned (uniquified if needed).
New filter using 'prefill'. Name auto-assigned (uniquified if needed).
Existing filter, user clears out name. New name auto-assigned (uniquified if needed).
Existing filter, user renames. User-assigned name is used (warned if a duplicate name).

Comment 10

[[e-mail]]

Comment on attachment 343319 [details] [diff] [review]
Updated patch

I wanted to suggest a few code tweaks:

>         gFilter = gFilterList.createFilter(gPreFillName);
>+        
>+        // Unfortunately the prefill code (in mailWindowOverlay.js) does not fill 'args' with 
>+        // separate 'filterName' and 'termValue' (or similar), and this code uses filterName 
>+        // for both the name of the filter, and for the match value (term.value).
>+        // We want to assign the name later in function AssignMeaningfulName(), so we need to
>+        // clear the filterName. 
>+        // One could wrap this line inside a pref check to revert to using the prefill
>+        // name for the filter instead of assigning a name below.
>+        gFilter.filterName = "";
Better still, just don't pass the prefill name in when creating the filter...

>+  AssignMeaningfulName();
I think it would be better to set filterName earlier, rather than changing gFilter.filterName afterwards.

>+    // Now name the filter, 'uniquifying' name.
>+    var stub = Term + " " + Operator + ": " + Value;
I'm concerned about localisation here.

Comment 11

[[e-mail]]

Comment on attachment 343319 [details] [diff] [review]
Updated patch

>+    var  termRoot = gSearchTerms[0].obj; // We only care about the first search term
These are the old search terms, you need to look at the current selection - gFilterActionList.getItemAtIndex(0)
You also have some unusual whitespace - two spaces between var and termRoot (and similar odd spacing in other places) plus 15 lines with trailing spaces.

Comment 12

[Matt Dudziak]

(In reply to comment #10)
> (From update of attachment 343319 [details] [diff] [review])
> I wanted to suggest a few code tweaks:
> 
> >         gFilter = gFilterList.createFilter(gPreFillName);
> >+        
> >+        // Unfortunately the prefill code (in mailWindowOverlay.js) does not fill 'args' with 
> >+        // separate 'filterName' and 'termValue' (or similar), and this code uses filterName 
> >+        // for both the name of the filter, and for the match value (term.value).
> >+        // We want to assign the name later in function AssignMeaningfulName(), so we need to
> >+        // clear the filterName. 
> >+        // One could wrap this line inside a pref check to revert to using the prefill
> >+        // name for the filter instead of assigning a name below.
> >+        gFilter.filterName = "";
> Better still, just don't pass the prefill name in when creating the filter...
> 

Can't not pass the prefill name as that is what is used (a few lines lower in the code) to set the 'termValue' (term.value). If we don't pass it in, the prefill code will essentially make a 'blank' filter (well, a 'From is <blank>' filter), which is not what we want.



> >+  AssignMeaningfulName();
> I think it would be better to set filterName earlier, rather than changing
> gFilter.filterName afterwards.
> 

Do you have a suggestion for where? It makes sense to me to wait until we are saving the filter, and have verified that all the actions are valid.

> >+    // Now name the filter, 'uniquifying' name.
> >+    var stub = Term + " " + Operator + ": " + Value;
> I'm concerned about localisation here.


What sort of concerns?

Comment 13

[Matt Dudziak]

(In reply to comment #11)
> (From update of attachment 343319 [details] [diff] [review])
> >+    var  termRoot = gSearchTerms[0].obj; // We only care about the first search term
> These are the old search terms, you need to look at the current selection -
> gFilterActionList.getItemAtIndex(0)

I'm not sure what you mean here. 
gFilterActionList appears to be the filter _actions_ which I am not interested in. I am only interested in the first search term.

As for being the 'old' search terms, I think this is OK. Just a few lines earlier, saveSearchTerms() was called so I am working with up-to-date information, and have the advantage of giving saveSearchTerms() a chance to 'scrub' the term (remove illegal characters, etc. if it wants) before I use it to create a name.

I did test the scenarios in comment #9 and this is working for me, so I am hesitant to make major changes if not needed.

> You also have some unusual whitespace - two spaces between var and termRoot
> (and similar odd spacing in other places) plus 15 lines with trailing spaces.

I'll fix the whitespace issues and once the other remaining questions have been resolved will submit another patch.

Thanks, Matt

Comment 14

[David :Bienvenu]

Comment on attachment 343319 [details] [diff] [review]
Updated patch

clearing review request

Comment 15

[Matt Dudziak]

Attachment: Updated patch that addresses whitespace issues and other problems.

The whitespace issues have been addressed. Also made a slight change to the way I get at the text so that it is simpler. Did not change any of the logic as there was never any response to my last comments and as I indicated previously I tested all the scenarios and it is working well for me.

Comment 16

[[e-mail]]

Comment on attachment 381183 [details] [diff] [review]
Updated patch that addresses whitespace issues and other problems.

>+  AssignMeaningfulName();
>+
>   // add each filteraction to the filter
>   for (index = 0; index < gFilterActionList.getRowCount(); index++)
>     gFilterActionList.getItemAtIndex(index).saveToFilter(gFilter);
> 
>   updateFilterType();
>   saveSearchTerms(gFilter.searchTerms, gFilter);
Sorry for confusing the search times with the action list, but I still think you're assigning the name before saving the search terms...

Comment 17

[[e-mail]]

(In reply to comment #12)
> (In reply to comment #10)
> > Better still, just don't pass the prefill name in when creating the filter...
> Can't not pass the prefill name as that is what is used (a few lines lower in
> the code) to set the 'termValue' (term.value). If we don't pass it in, the
> prefill code will essentially make a 'blank' filter (well, a 'From is <blank>'
> filter), which is not what we want.
Sure, still set term.value, just (imho) no point passing it to createFilter.

> > I think it would be better to set filterName earlier, rather than changing
> > gFilter.filterName afterwards.
> Do you have a suggestion for where? It makes sense to me to wait until we are
> saving the filter, and have verified that all the actions are valid.
As I recall, I was thinking of doing it in place of alerting for a blank name.

> > >+    // Now name the filter, 'uniquifying' name.
> > >+    var stub = Term + " " + Operator + ": " + Value;
> > I'm concerned about localisation here.
> What sort of concerns?
My understanding is that different locales can have different word ordering. Without knowing that this is safe I can only assume that it is unsafe.

Comment 18

[[e-mail]]

(In reply to comment #15)
> Did not change any of the logic as there was never any response to my last
> comments and as I indicated previously I tested all the scenarios and it is
> working well for me.
Sorry, I wasn't on the CC list so I hadn't seen your comments.

Comment 19

[Matt Dudziak]

(In reply to comment #16)
> (From update of attachment 381183 [details] [diff] [review])
> >+  AssignMeaningfulName();
> >+
> >   // add each filteraction to the filter
> >   for (index = 0; index < gFilterActionList.getRowCount(); index++)
> >     gFilterActionList.getItemAtIndex(index).saveToFilter(gFilter);
> > 
> >   updateFilterType();
> >   saveSearchTerms(gFilter.searchTerms, gFilter);
> Sorry for confusing the search times with the action list, but I still think
> you're assigning the name before saving the search terms...


OK, I've moved the call to assign the name after saving. Will appear in next patch. I thought there was a reason I put it where it was, but so far I am not seeing any problem with it where I have moved it.

Comment 20

[Matt Dudziak]

(In reply to comment #17)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Better still, just don't pass the prefill name in when creating the filter...
> > Can't not pass the prefill name as that is what is used (a few lines lower in
> > the code) to set the 'termValue' (term.value). If we don't pass it in, the
> > prefill code will essentially make a 'blank' filter (well, a 'From is <blank>'
> > filter), which is not what we want.
> Sure, still set term.value, just (imho) no point passing it to createFilter.
> 


Are you are suggesting that I change ~line 317 
from:
    // This is a new filter
    gFilter = gFilterList.createFilter(filterName);
to:
    // This is a new filter
    gFilter = gFilterList.createFilter();

If so, that will not work. Eudora (Thunderbird) goes off into the weeds if the parameter is not there. If this is not what you mean, please elaborate. Thanks.

Matt

Comment 21

[Matt Dudziak]

> > > >+    // Now name the filter, 'uniquifying' name.
> > > >+    var stub = Term + " " + Operator + ": " + Value;
> > > I'm concerned about localisation here.
> > What sort of concerns?
> My understanding is that different locales can have different word ordering.
> Without knowing that this is safe I can only assume that it is unsafe.

OK, I've changed the code to use a format string from the string bundle and have added comments to the string bundle to assist localizers. Changes will appear in next patch.

Comment 22

[Matt Dudziak]

(In reply to comment #17)
> (In reply to comment #12)
> > (In reply to comment #10)

> > > I think it would be better to set filterName earlier, rather than changing
> > > gFilter.filterName afterwards.
> > Do you have a suggestion for where? It makes sense to me to wait until we are
> > saving the filter, and have verified that all the actions are valid.
> As I recall, I was thinking of doing it in place of alerting for a blank name.


Sorry, but I am a bit confused here. On one hand you are suggesting that I assign the name earlier, but at the same time you are concerned that I am assigning the name before saving the search terms. I've moved the call to "AssignMeaningfuleName()" below the save:
  updateFilterType();
  saveSearchTerms(gFilter.searchTerms, gFilter);
  AssignMeaningfulName();

Where are you suggesting that I move it? I think if we can get this item figured out I can submit the patch again.

Thanks, Matt

Comment 23

[[e-mail]]

(In reply to comment #20)
>     gFilter = gFilterList.createFilter();
> 
> Thunderbird goes off into the weeds if the parameter is not there.
You can probably pass null or "" or something, but maybe I can persuade bienvenu to allow the parameter to be made optional.

> Sorry, but I am a bit confused here. On one hand you are suggesting that I
> assign the name earlier, but at the same time you are concerned that I am
> assigning the name before saving the search terms.
It wasn't originally obvious to me not to assign the name earlier because you were (erroneously) doing it before saving the search terms. My only issue with doing it after saving the search terms was that when you create a new filter you don't necessarily know what the name you intend to use is yet. If you think it's better to read the saved search terms then will stick with your way.

I'm not a fan of having the body of AssignMeaningfulName wrapped in a big if statement, when it would be possible to make the caller do the check e.g.
if (!filterName)
  gFilter.filterName = GenerateMeaningfulName(gFilter.searchTerms[0].obj);

Oh, and stub + " " + count; works without the .toString()

Comment 24

[David :Bienvenu]

(In reply to comment #23)
> (In reply to comment #20)
> >     gFilter = gFilterList.createFilter();
> > 
> > Thunderbird goes off into the weeds if the parameter is not there.
> You can probably pass null or "" or something, but maybe I can persuade
> bienvenu to allow the parameter to be made optional.

Sure, you can make it optional if you want...

Comment 25

[Matt Dudziak]

Attachment: Latest Patch

Adresses whitespace issues and the latest comments. Will also submit an attachment that is the patch without the whitespace changes as that makes it much easier to read.

Comment 26

[Matt Dudziak]

Attachment: Latest patch without whitespace changes.

A copy of the latest patch, just without the whitespace changes.

Comment 27

[[e-mail]]

(In reply to comment #13)
> As for being the 'old' search terms, I think this is OK. Just a few lines
> earlier, saveSearchTerms() was called so I am working with up-to-date
> information, and have the advantage of giving saveSearchTerms() a chance to
> 'scrub' the term (remove illegal characters, etc. if it wants) before I use it
> to create a name.
Hmm, so what confused me is that you're actually reading the search terms from the UI all along... the search widgets don't currently validate, which is why you didn't notice that your code was working anyway.

Comment 28

[[e-mail]]

Comment on attachment 382353 [details] [diff] [review]
Latest Patch

>diff -r e2b8a59771f9 mail/locales/en-US/chrome/messenger/filter.properties
Nit: suite/locales/en-US/chrome/mailnews/filter.properties will need an update.

>+// The actual filter that we're editing if it is a _saved_ filter; void otherwise:
Or prefill.

>         gFilter = gFilterList.createFilter(gPreFillName);
You didn't try createFilter(null)?

>-      name = stub + " " + count.toString();
Aha!

>   // if we made it here, all of the actions are valid, so go ahead and save the filter
At this point you know the filter is valid, and because you read the value from the UI you don't need to call saveSearchTerms to read it, so you could generate the filter name now before creating the filter.

>+  // Values are either popup menu items or edit fields.
>+  // For popup menus use activeItem.label; for
>+  // edit fields, activeItem.value
>+  var Value;
Nit: variable names should begin with a lower case letter

>+  switch (Number(termRoot.searchattribute.value))
Might it be better to switch on the activeItem's localName?

Comment 29

[Matt Dudziak]

(In reply to comment #28)
> (From update of attachment 382353 [details] [diff] [review])
> >diff -r e2b8a59771f9 mail/locales/en-US/chrome/messenger/filter.properties
> Nit: suite/locales/en-US/chrome/mailnews/filter.properties will need an update.

OK, will do.


> >+// The actual filter that we're editing if it is a _saved_ filter; void otherwise:
> Or prefill.

OK, added prefill

> 
> >         gFilter = gFilterList.createFilter(gPreFillName);
> You didn't try createFilter(null)?
> 

That isn't my code. That is not a line I added. I just added all the comments below it.

> >-      name = stub + " " + count.toString();
> Aha!

 Yep. Works fine. Thanks.

> 
> >   // if we made it here, all of the actions are valid, so go ahead and save the filter
> At this point you know the filter is valid, and because you read the value from
> the UI you don't need to call saveSearchTerms to read it, so you could generate
> the filter name now before creating the filter.

Again, I didn't add this code. I added a comment and made a change (passing nothing to createFilter) per your suggestion:

-    gFilter = gFilterList.createFilter(filterName);
+    // This is a new filter
+    gFilter = gFilterList.createFilter("");

I'm hesitant to go changing existing code as part of this patch, unless there is good reason. I think it would be best to use another patch for various changes unrelated to the filter naming code I've added.
> 
> >+  // Values are either popup menu items or edit fields.
> >+  // For popup menus use activeItem.label; for
> >+  // edit fields, activeItem.value
> >+  var Value;
> Nit: variable names should begin with a lower case letter

Fixed. Next 

> >+  switch (Number(termRoot.searchattribute.value))
> Might it be better to switch on the activeItem's localName?

This was by far the hardest part of this change; deciding which value to switch. After trying many different things that failed for various reasons, we decided on termRoot.searchattribute.value (which works in all cases). Unless you have a strong reason to change, I'd rather not switch (pardon the pun) at this point.


I'll submit another patch incorporating the latest changes.

Matt

Comment 30

[Matt Dudziak]

Attachment: 11June_patch

Patch updated 11June to reflect the latest changes suggested by Neil.

Comment 31

[Matt Dudziak]

Attachment: 11June patch without whitespace changes displayed

This is the same as the 11June patch, just used the '-w' option so that whitespace changes are ignored. Makes it easier to read that way.

Comment 32

[[e-mail]]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 382353 [details] [diff] [review])
> > >         gFilter = gFilterList.createFilter(gPreFillName);
> > You didn't try createFilter(null)?
> That isn't my code. That is not a line I added. I just added all the comments
> below it.
...
> I'm hesitant to go changing existing code as part of this patch, unless there
> is good reason.
Changing existing code is better than working around it.

> > >   // if we made it here, all of the actions are valid, so go ahead and save the filter
> > At this point you know the filter is valid, and because you read the value from
> > the UI you don't need to call saveSearchTerms to read it, so you could generate
> > the filter name now before creating the filter.
> Again, I didn't add this code. I added a comment and made a change (passing
> nothing to createFilter) per your suggestion:
I'm just suggesting that you could generate the filter name here, so that it's ready for when you create the filter.

> > >+  switch (Number(termRoot.searchattribute.value))
> > Might it be better to switch on the activeItem's localName?
> This was by far the hardest part of this change; deciding which value to
> switch. After trying many different things that failed for various reasons, we
> decided on termRoot.searchattribute.value (which works in all cases). Unless
> you have a strong reason to change, I'd rather not switch (pardon the pun) at
> this point.
Yeah, maybe we should have a followup to fix this in the XBL.

Comment 33

[Matt Dudziak]

(In reply to comment #32)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (From update of attachment 382353 [details] [diff] [review])
> > > >         gFilter = gFilterList.createFilter(gPreFillName);
> > > You didn't try createFilter(null)?
> > That isn't my code. That is not a line I added. I just added all the comments
> > below it.
> ...
> > I'm hesitant to go changing existing code as part of this patch, unless there
> > is good reason.
> Changing existing code is better than working around it.
> 
> > > >   // if we made it here, all of the actions are valid, so go ahead and save the filter
> > > At this point you know the filter is valid, and because you read the value from
> > > the UI you don't need to call saveSearchTerms to read it, so you could generate
> > > the filter name now before creating the filter.
> > Again, I didn't add this code. I added a comment and made a change (passing
> > nothing to createFilter) per your suggestion:
> I'm just suggesting that you could generate the filter name here, so that it's
> ready for when you create the filter.
> 
> > > >+  switch (Number(termRoot.searchattribute.value))
> > > Might it be better to switch on the activeItem's localName?
> > This was by far the hardest part of this change; deciding which value to
> > switch. After trying many different things that failed for various reasons, we
> > decided on termRoot.searchattribute.value (which works in all cases). Unless
> > you have a strong reason to change, I'd rather not switch (pardon the pun) at
> > this point.
> Yeah, maybe we should have a followup to fix this in the XBL.


OK. Fixed in the next patch.

Comment 34

[Matt Dudziak]

(In reply to comment #32)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (From update of attachment 382353 [details] [diff] [review])
> > > >         gFilter = gFilterList.createFilter(gPreFillName);
> > > You didn't try createFilter(null)?
> > That isn't my code. That is not a line I added. I just added all the comments
> > below it.
> ...
> > I'm hesitant to go changing existing code as part of this patch, unless there
> > is good reason.
> Changing existing code is better than working around it.
> 


Please ignore comment #33. It was not supposed to refer to all the quoted text; just the text quoted here. This is fixed in the next patch.

Comment 35

[Matt Dudziak]

> > >   // if we made it here, all of the actions are valid, so go ahead and save the filter
> > At this point you know the filter is valid, and because you read the value from
> > the UI you don't need to call saveSearchTerms to read it, so you could generate
> > the filter name now before creating the filter.
> Again, I didn't add this code. I added a comment and made a change (passing
> nothing to createFilter) per your suggestion:
I'm just suggesting that you could generate the filter name here, so that it's
ready for when you create the filter.


I can't assign the name at this point as in most cases gFilter does not exist yet so the naming code goes off into the weeds. That said, I can and have moved the call to AssignMeaningfulName up a few lines (just before saveSearchTerms(gFilter.searchTerms, gFilter);)

Will appear in next patch.

Comment 36

[Matt Dudziak]

Attachment: Latest Patch

Latest patch incorporating comments #32-35. Will also upload a patch ignoring whitespace.

Comment 37

[Matt Dudziak]

Attachment: Latest patch without whitespace changes.

16Jun patch without whitespace changes

Comment 38

[[e-mail]]

Comment on attachment 383532 [details] [diff] [review]
Latest Patch

I only just noticed that "Match all messages" filters get an odd name...

-    gFilterContext.selectedIndex = 2; // both
-  }
+  gFilterContext.selectedIndex = 2; // both
This is wrong; we don't want to do this for an existing filter.

Ironically you readded trailing whitespace to two of your lines...

>-        gFilter = gFilterList.createFilter(gPreFillName);
>+        

>+  // For localization purposes, use a string format in the string bundle
>+

Comment 39

[Jeff Beckley]

(In reply to comment #38)
> (From update of attachment 383532 [details] [diff] [review])
> I only just noticed that "Match all messages" filters get an odd name...
> 
> -    gFilterContext.selectedIndex = 2; // both
> -  }
> +  gFilterContext.selectedIndex = 2; // both
> This is wrong; we don't want to do this for an existing filter.

Right.  Only new filters should get defaulted to both the incoming and manual filter contexts.

Comment 40

[Matt Dudziak]

(In reply to comment #38)
> (From update of attachment 383532 [details] [diff] [review])
> I only just noticed that "Match all messages" filters get an odd name...
> 
> -    gFilterContext.selectedIndex = 2; // both
> -  }
> +  gFilterContext.selectedIndex = 2; // both
> This is wrong; we don't want to do this for an existing filter.

Good catch! I hadn't tested that scenario.
Fixed.

> 
> Ironically you readded trailing whitespace to two of your lines...
> 
Aarrrrgghh ;-)
Fixed.

Comment 41

[Matt Dudziak]

Attachment: Patch with latest fixes (problems from comment #38)

Patch that fixes the problems pointed out in comment #38. Also split some comments to two lines to avoid lines that were >80 chars.

Comment 42

[Matt Dudziak]

Attachment: No whitespace version of the 19 June patch

Version of the 19 June patch that ignores whitespace differences.

Comment 43

[[e-mail]]

Sorry for the delay, but I've had a busy weekend and it took me some time to catch up.

I think you overlooked one of my comments on the previous patch: what if you create a filter that matches all messages?

Comment 44

[Matt Dudziak]

(In reply to comment #43)
> Sorry for the delay, but I've had a busy weekend and it took me some time to
> catch up.
> 
> I think you overlooked one of my comments on the previous patch: what if you
> create a filter that matches all messages?

Yes, I did overlook that. I was on vacation for 2 weeks then had problems with Venkman, so just now got around to looking at this.

I have addressed this in the patch I will submit in a few minutes. 


Matt

Comment 45

[Matt Dudziak]

Attachment: Latest Patch, now dealing with 'Match all messages' filters

Comment 46

[Matt Dudziak]

Attachment: No whitespace version of the 15 July patch

A no whitespace version of the patch for easier reading.

Comment 47

[[e-mail]]

Comment on attachment 388823 [details] [diff] [review]
Latest Patch, now dealing with 'Match all messages' filters

>+  var termRoot = gSearchTerms[0].obj; 
Nit: a trailing space crept in here somehow

>+        // We should never get here, but for safety's sake,
>+        // let's name the filter "Untitled Filter".
>+        gFilter.filterName = gFilterBundle.getString( "untitledFilterName" );
>+        return;
>+      }
>+      break;
>+  }
>+
>+  // Now name the filter, 'uniquifying' name.
>+  // Have one special case to handle. If this is a 'Match all messages' filter,
>+  // we need to give it a special name that is NOT based on the search strings.
>+  // Otherwise, for localization purposes, use a string format in the string bundle
>+  var stub;
>+  if (gSearchTerms[0].obj.matchAll)
>+  {
>+    stub = gFilterBundle.getString( "matchAllFilterName" );
Could we some how accidentally return early above and therefore not get here, when we don't actually care about the search row since we're matching all? (And should we care about uniquifying an untitled filter?)

>+  
And one here too ;-)

Comment 48

[David :Bienvenu]

Comment on attachment 388824 [details]
No whitespace version of the 15 July patch

+// The actual filter that we're editing if it is a _saved_ filter or prefill;
+// void otherwise:

and

+// The filter name as it appears in the "Filter Name" field of dialog:

should end sentences with ".".

typo - new filter.

+    // This is a new fiter. Set to both Incoming and Manual contexts
+    gFilterContext.selectedIndex = 2;

no need for braces here:

+  if (!gFilter.filterName)
+  {
+    AssignMeaningfulName();
+  }

don't need temp var array here:

+  else
+  {
+    var array = [term, operator, value];
+    stub = gFilterBundle.getFormattedString("filterAutoNameStr", array);
+  }
+

why not use ++count here, instead of separate count++ statement:

+  {
+    count++;
+    tempName = stub + " " + count;
+  }

finally, can you use let instead of var in AssignMeaningfulName()? 

sr=me, with those, and Neil's comments addressed, but might as well request sr on a new patch...

Comment 49

[David :Bienvenu]

Comment on attachment 388823 [details] [diff] [review]
Latest Patch, now dealing with 'Match all messages' filters

minusing just to get off my queue...

Comment 50

[Matt Dudziak]

(In reply to comment #47)
> (From update of attachment 388823 [details] [diff] [review])
> >+  var termRoot = gSearchTerms[0].obj; 
> Nit: a trailing space crept in here somehow
> 
> >+        // We should never get here, but for safety's sake,
> >+        // let's name the filter "Untitled Filter".
> >+        gFilter.filterName = gFilterBundle.getString( "untitledFilterName" );
> >+        return;
> >+      }
> >+      break;
> >+  }
> >+
> >+  // Now name the filter, 'uniquifying' name.
> >+  // Have one special case to handle. If this is a 'Match all messages' filter,
> >+  // we need to give it a special name that is NOT based on the search strings.
> >+  // Otherwise, for localization purposes, use a string format in the string bundle
> >+  var stub;
> >+  if (gSearchTerms[0].obj.matchAll)
> >+  {
> >+    stub = gFilterBundle.getString( "matchAllFilterName" );
> Could we some how accidentally return early above and therefore not get here,
> when we don't actually care about the search row since we're matching all? (And
> should we care about uniquifying an untitled filter?)
> 
> >+  
> And one here too ;-)


OK, fixed the tailing spaces.

As for the other question, I don't see how we would accidentally return early above, though I suppose anything is possible. Do YOU see how we might? As for uniquifying an untitled filter, I don't think we NEED to but the original code did.

Comment 51

[Matt Dudziak]

(In reply to comment #48)
> (From update of attachment 388824 [details])
> +// The actual filter that we're editing if it is a _saved_ filter or prefill;
> +// void otherwise:
> 
> and
> 
> +// The filter name as it appears in the "Filter Name" field of dialog:
> 
> should end sentences with ".".

OK, fixed. For the record there are plenty of other comments (most of them?) in this file that do not end with ".".

> 
> typo - new filter.
> 
> +    // This is a new fiter. Set to both Incoming and Manual contexts
> +    gFilterContext.selectedIndex = 2;
> 

fixed

> no need for braces here:
> 
> +  if (!gFilter.filterName)
> +  {
> +    AssignMeaningfulName();
> +  }
> 

Fixed

> don't need temp var array here:
> 
> +  else
> +  {
> +    var array = [term, operator, value];
> +    stub = gFilterBundle.getFormattedString("filterAutoNameStr", array);
> +  }
> +
> 

Changed.
> why not use ++count here, instead of separate count++ statement:
> 
> +  {
> +    count++;
> +    tempName = stub + " " + count;
> +  }
> 

I though about it, but then this:

  while (duplicateFilterNameExists(tempName))
  {
    count++;
    tempName = stub + " " + count;
  }

turns into this:

  while (duplicateFilterNameExists(tempName))
    tempName = stub + " " + ++count;

which just does not seem as readable. The   " " + ++count    is odd.

Also, as it turns out, the way I am doing it I copied from the original code for assigning unique names to untitled filters so I am basically re-using legacy code...

If you feel strongly I will change it but I think it is more readable the way it is now.


> finally, can you use let instead of var in AssignMeaningfulName()? 

Yes, I think I can. Changing.
> 
> sr=me, with those, and Neil's comments addressed, but might as well request sr
> on a new patch...

Comment 52

[Matt Dudziak]

(In reply to comment #47)
> (From update of attachment 388823 [details] [diff] [review])
> >+  var termRoot = gSearchTerms[0].obj; 
> Nit: a trailing space crept in here somehow
> 
> >+        // We should never get here, but for safety's sake,
> >+        // let's name the filter "Untitled Filter".
> >+        gFilter.filterName = gFilterBundle.getString( "untitledFilterName" );
> >+        return;
> >+      }
> >+      break;
> >+  }
> >+
> >+  // Now name the filter, 'uniquifying' name.
> >+  // Have one special case to handle. If this is a 'Match all messages' filter,
> >+  // we need to give it a special name that is NOT based on the search strings.
> >+  // Otherwise, for localization purposes, use a string format in the string bundle
> >+  var stub;
> >+  if (gSearchTerms[0].obj.matchAll)
> >+  {
> >+    stub = gFilterBundle.getString( "matchAllFilterName" );
> Could we some how accidentally return early above and therefore not get here,
> when we don't actually care about the search row since we're matching all? (And
> should we care about uniquifying an untitled filter?)



I'm refactoring this code to better handle this situation.....

Comment 53

[Matt Dudziak]

Attachment: Patch addressing comments and refactoring

This patch addresses (I believe) all the comments and includes some refactoring since the previous patch.

Comment 54

[David :Bienvenu]

Comment on attachment 391232 [details] [diff] [review]
Patch addressing comments and refactoring

sr=bienvenu, with a nit (in two files) :

+# %3$S=Value, ie. "Steve Jobs", "Important", "42", etc.

can you use e.g. instead of i.e. here?

thx for the patch!

I'm not sure Neil wanted to review a new patch since he said you might as well ask for sr on the new patch, not r, but I'll let him decide.

Comment 55

[[e-mail]]

Comment on attachment 391232 [details] [diff] [review]
Patch addressing comments and refactoring

The refactoring looks OK to me although I didn't test it this time.

>+  if (termRoot.matchAll)
>+      stub = gFilterBundle.getString( "matchAllFilterName" );
Nit: should be 2-space indent, I think?

Comment 56

[Matt Dudziak]

(In reply to comment #54)
> (From update of attachment 391232 [details] [diff] [review])
> sr=bienvenu, with a nit (in two files) :
> 
> +# %3$S=Value, ie. "Steve Jobs", "Important", "42", etc.
> 
> can you use e.g. instead of i.e. here?
> 
> thx for the patch!
> 
> I'm not sure Neil wanted to review a new patch since he said you might as well
> ask for sr on the new patch, not r, but I'll let him decide.


OK, changed 6 occurrences (3 in each file) of 'ie.' to 'e.g.'

Matt

Comment 57

[Matt Dudziak]

(In reply to comment #55)
> (From update of attachment 391232 [details] [diff] [review])
> The refactoring looks OK to me although I didn't test it this time.
> 
> >+  if (termRoot.matchAll)
> >+      stub = gFilterBundle.getString( "matchAllFilterName" );
> Nit: should be 2-space indent, I think?

Ugh. Yes, it should be :-) Fixed. 

I'll wait a few hours and if I do not see any other comments here will submit another patch.

Matt

Comment 58

[Matt Dudziak]

Attachment: Patch to address latest comments (even though last patch was approved)

Even though you both approved the last patch, I addressed the comments you made:

Changed 6 occurrences (3 in each of 2 files) of 'ie.' to 'e.g.'
Removed 2 extra spaces before second line here:
>+  if (termRoot.matchAll)
>+      stub = gFilterBundle.getString( "matchAllFilterName" );

Hopefully this will be the last of the changes required for this patch.

Comment 59

[David :Bienvenu]

Comment on attachment 391716 [details] [diff] [review]
Patch to address latest comments (even though last patch was approved)

thx, Matt.

Comment 60

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/4d44899c5706

Comment 61

[Kent James (:rkent)]

After this patch, when I create a new filter and assign it a name, that name is not used, instead it is overwritten with the auto-assigned name. According to comment 9 ("New filter, user assigns name. User-assigned name is used") that is not the expected behaviour. Was there any change to that expectation?

Comment 62

[Matt Dudziak]

(In reply to comment #61)
> After this patch, when I create a new filter and assign it a name, that name is
> not used, instead it is overwritten with the auto-assigned name. According to
> comment 9 ("New filter, user assigns name. User-assigned name is used") that is
> not the expected behaviour. Was there any change to that expectation?

Aargh. It used to work correctly. This bug must have crept in during the refactoring. I'll fix this, but in the mean time, you can go back and rename the filter after its name has been auto-assigned (filter saved) and your name will stick.

Matt

Comment 63

[Kent James (:rkent)]

(In reply to comment #62)

> Aargh.

I feel your pain. As another person who has worked on this code, I find it very difficult to change reliably.

Comment 64

[[e-mail]]

I failed to test this on a manual pop account filter and the attachment and junk status rules don't get described correctly ("Junk Status is: 2").

Comment 65

[Matt Dudziak]

(In reply to comment #62)
> (In reply to comment #61)
> > After this patch, when I create a new filter and assign it a name, that name is
> > not used, instead it is overwritten with the auto-assigned name. According to
> > comment 9 ("New filter, user assigns name. User-assigned name is used") that is
> > not the expected behaviour. Was there any change to that expectation?
> 
> Aargh. It used to work correctly. This bug must have crept in during the
> refactoring. I'll fix this, but in the mean time, you can go back and rename
> the filter after its name has been auto-assigned (filter saved) and your name
> will stick.
> 
> Matt


I KNEW there was a reason not to change

gFilter = gFilterList.createFilter(filterName);
to:
gFilter = gFilterList.createFilter(null);

Comment 66

[Matt Dudziak]

(In reply to comment #64)
> I failed to test this on a manual pop account filter and the attachment and
> junk status rules don't get described correctly ("Junk Status is: 2").

OK, I have this fixed. 

I also changed behavior for the Junk Score Origin Case, as I prefer the capitalized 'Plugin', 'User', 'Filter', Whitelist' and 'IMAP Flag'. activeitem.label gives me the Capitalized version while activeitem.value gives me the lower-case version (e.g. 'Plugin' vs. 'plugin')


Neil, what is the best way to move forward here? Do I submit another patch against this bug, or do I create a new bug for the problems addressed and submit the patch there?

Thanks, Matt.

Comment 67

[Magnus Melin [:mkmelin]]

New bug, mark it blocking this one.

Comment 68

[Matt Dudziak]

Patch submitted to https://bugzilla.mozilla.org/show_bug.cgi?id=510703