1328855 - Message filter not added to open "Message Filter" dialogue

Status: RESOLVED FIXED

(Thunderbird :: Filters, defect)

Description

[DutchGemini]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.87 Safari/537.36

Steps to reproduce:

1. Open the Message Filters dialogue.
2. Create a message filter from a sender's address via the header of the message preview.



Actual results:

In the open "Message Filters" dialogue the newly added filter is not listed.

When trying to enter it again, an error is triggered about duplication, so the filter is apparently there but not visible.

Indeed, if you enter [part of] the filter's name in search filter text box of the same dialogue, the filter magically appears. Also closing and re-opening lists the new filter.

Looks like a missing refresh.

Note: on the dialogue there is no 'refresh' function.


Expected results:

Newly added filters shall automatically update the open "Message Filters" dialogue.

Comment 1

[:aceman]

Is there anything in Tools->error console after you do this?

I remember I fixed stuff like this in the Filter list dialog, e.g. in bug 543419.

Comment 2

[Jorg K (CEST = GMT+2)]

Confirmed on trunk. I added filter "AAA" and it doesn't show up in the list. When you search for it, it is there. Nothing in the error console.

Comment 3

[DutchGemini]

Nothing in error console.

Comment 5

[[e-mail]]

I finally ended up fiddling around with the plist file, edited it in text edit (copied and pasted a working filter then changed the necessary data in it such as name and constraints and destinations), saved under another name in another place, got to finder, deleted the existing plist file, moved the newly created from the place it was stored to the profile, and here we are.

for me it is fixed, but not solved.

Comment 6

[:aceman]

I do see it now.

Comment 7

[:aceman]

I think this got regressed by bug 1025373 or bug 363982.

Comment 8

[:aceman]

Attachment: patch

I see this patch got a bit over the top. But I couldn't get my head around what gCurrentFolder and gSelectedFolder variables actually contain. I think that was the reason the if() evaluated wrongly and didn't take the path to execute rebuildFilterList. Also gSelectedFolder was used to pass values from one function to another. So I remove those dubious variables and now reliably store the selected values in ._folder members of the respective pickers (menulist elements). I removed some unneeded and ambiguously named (select*folder/set*folder) functions that were calling each other and initialized state differently depending on whether selecting a new server/folder was done by user via the picker or via setup functions. The code paths should now be merged/shared and the state in ._folder set reliably.

In this setup it is easier to decide whether to change everything on call to refresh() or just rebuild the list.

Alta, can you check his a bit and see if the "Choose Folder" item is still selected properly depending on account type?

Comment 9

[:aceman]

Comment on attachment 8841799 [details] [diff] [review]
patch

RealRaven, I think you have an addon that overlays this filter list dialog. Can you please check if I am not missing some usecase and remove something you critically need? Thanks.

Comment 10

[alta88]

Comment on attachment 8841799 [details] [diff] [review]
patch


Looks like it's working correctly for all acct types, in all variations of folderpane selection, plus is a lot more logical code wise than before.

Comment 11

[alta88]

Oops, sorry, in the case of no folderpane selection, you don't handle it cleanly.  The Tools/Appmenu menuitems should be disabled if there is no selection in folderpane.

Comment 12

[Axel Grude [:realRaven]]

(In reply to :aceman from comment #9)
> Comment on attachment 8841799 [details] [diff] [review]
> patch
> 
> RealRaven, I think you have an addon that overlays this filter list dialog.
> Can you please check if I am not missing some usecase and remove something
> you critically need? Thanks.

Well I was using onFilterFolderClick( ) but it does fall back to 
   onFilterServerClick( ) and then
   serverPopup.selectFolder( )

from what I see in my comments below using serverPopup.selectFolder( ) did not refresh the list anymore, but that might be fixed in the meantime. See code snippete below

I have quite extensive (across servers, based on target folder, search all text conditions, search by tag action, search by reply with template) search options in quickfilters, so I will have to do some intensive testing. It's probably a good idea if I do a full set of sanity tests, including the copy/paste/sort functions. I even have an option to add a new filter in alphabetical position.

Could you point me to a test build?

relevant code snippet referencing onFilterFolderClick( ). Since it is suite / postbox compatible it may fall back gracefully. 

  selectFoundFilter: function selectFoundFilter(el) {
    quickFilters.List.toggleSearchType('targetFolder');
    document.getElementById('quickFiltersSearchTargetFolder').setAttribute('checked','true');
    
    // find out of we need to change server:
    let item = el.selectedItem,
        account = item.targetAccount,
        targetFilter = item.targetFilter,
        uri = item.getAttribute('targetFolderUri'),
        actionType = item.getAttribute('actionType'),    
    // change server to correct account
        aFolder = account ? account.rootMsgFolder : null;
    if (typeof onFilterFolderClick !== 'undefined') {
      //Thunderbird specific code!
      onFilterFolderClick(aFolder);
    }
    else {
      if (typeof onFilterServerClick !== 'undefined') {
        onFilterServerClick(aFolder.URI); // suite
      }
      else {
        let serverPopup = quickFilters.List.ServerMenuPopup;
        serverPopup.selectFolder(aFolder); // this didn't rebuild anymore!
        if (quickFilters.Util.Application === 'SeaMonkey') {
          quickFilters.List.rebuildFilterList();
        }
      }
    }
  
    // select found filter
    quickFilters.List.selectFilter(targetFilter);
    
  }

Comment 13

[:aceman]

(In reply to alta88 from comment #11)
> Oops, sorry, in the case of no folderpane selection, you don't handle it
> cleanly.
Good catch, fixed now.

> The Tools/Appmenu menuitems should be disabled if there is no
> selection in folderpane.
That woud be for a different bug, but we select the default account when no folder/server was asked for so I keep that now.

Comment 14

[:aceman]

(In reply to Axel Grude [:realRaven] from comment #12)
> Well I was using onFilterFolderClick( ) but it does fall back to 
>    onFilterServerClick( ) and then
>    serverPopup.selectFolder( )
> 
> from what I see in my comments below using serverPopup.selectFolder( ) did
> not refresh the list anymore, but that might be fixed in the meantime. See
> code snippete below

If the serverPopup is the menupopup with type="folder" then its .selectFolder() does not refresh any list.
It just selects a folder in the widget and sets up the label of the menulist.

> Could you point me to a test build?

Yes, see https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ee8295fd7086ea9db1833c5d44465099b13afffb

> relevant code snippet referencing onFilterFolderClick( ). Since it is suite
> / postbox compatible it may fall back gracefully. 

You may test for the new setFilterFolder, unless it will be deemed to keep the name 'onFilterFolderClick' instead. But that name was marked incorrect even before the patch.

Comment 15

[Axel Grude [:realRaven]]

(In reply to :aceman from comment #14)
> (In reply to Axel Grude [:realRaven] from comment #12)
> > Well I was using onFilterFolderClick( ) but it does fall back to 
> >    onFilterServerClick( ) and then
> >    serverPopup.selectFolder( )
> > 
> > from what I see in my comments below using serverPopup.selectFolder( ) did
> > not refresh the list anymore, but that might be fixed in the meantime. See
> > code snippete below
> 
> If the serverPopup is the menupopup with type="folder" then its
> .selectFolder() does not refresh any list.
> It just selects a folder in the widget and sets up the label of the menulist.
> 

I managed to download + run the test build you pointed to and indeed it goes as far as 

serverPopup.selectFolder(aFolder)

        serverPopup.selectFolder(aFolder); // this didn't rebuild anymore!
-       if (quickFilters.Util.Application === 'SeaMonkey') {
+       if (quickFilters.Util.Application !== 'Postbox') {
          quickFilters.List.rebuildFilterList();
        }

[like it would in SeaMonkey] - so then I am forcing a refresh of the list using this:

rebuildFilterList(gCurrentFilterList);

but that's not working because gCurrentFilterList is stilll pointing to the list on the previous server. What else do I need to do to rebuild the list according to the server change?

Comment 16

[Axel Grude [:realRaven]]

(In reply to Axel Grude [:realRaven] from comment #15)
> (In reply to :aceman from comment #14)
> > (In reply to Axel Grude [:realRaven] from comment #12)
> > > Well I was using onFilterFolderClick( ) but it does fall back to 
> > >    onFilterServerClick( ) and then
> > >    serverPopup.selectFolder( )
> > > 
> > > from what I see in my comments below using serverPopup.selectFolder( ) did
> > > not refresh the list anymore, but that might be fixed in the meantime. See
> > > code snippete below
> > 
> > If the serverPopup is the menupopup with type="folder" then its
> > .selectFolder() does not refresh any list.
> > It just selects a folder in the widget and sets up the label of the menulist.
> > 
> 
> I managed to download + run the test build you pointed to and indeed it goes
> as far as 
> 
> serverPopup.selectFolder(aFolder)
> 
>         serverPopup.selectFolder(aFolder); // this didn't rebuild anymore!
> -       if (quickFilters.Util.Application === 'SeaMonkey') {
> +       if (quickFilters.Util.Application !== 'Postbox') {
>           quickFilters.List.rebuildFilterList();
>         }
> 
> [like it would in SeaMonkey] - so then I am forcing a refresh of the list
> using this:
> 
> rebuildFilterList(gCurrentFilterList);
> 
> but that's not working because gCurrentFilterList is stilll pointing to the
> list on the previous server. What else do I need to do to rebuild the list
> according to the server change?

Fixed it:

        if (util.Application !== 'Postbox') {
          // rebuild list in case of server change
+	  if (typeof gCurrentFilterList !== 'undefined')
+	    gCurrentFilterList = aFolder.getEditableFilterList(gFilterListMsgWindow);
          qList.rebuildFilterList();
        }

(there is already special code in rebuildFilterList() for Seamonkey.)

Now I need to find again how to reset that feedback flag...

Comment 17

[:aceman]

(In reply to Axel Grude [:realRaven] from comment #16)
> Fixed it:
> 
>         if (util.Application !== 'Postbox') {
>           // rebuild list in case of server change
> +	  if (typeof gCurrentFilterList !== 'undefined')
> +	    gCurrentFilterList =
> aFolder.getEditableFilterList(gFilterListMsgWindow);
>           qList.rebuildFilterList();
>         }
> 

This looks like the code in setFolder(). Can you call that?
In the patch it is renamed to setFilterFolder.

Comment 18

[Axel Grude [:realRaven]]

I set the feedback from ? to +  via Patch > Details... I hope I did this right, I couldn't find any instructions on how to approve it. 

Anyway, approving the patch from my perspective, got my own patch in place and it's backwards compatible. I may call setFilterFolder() instead if it is defined as alternative. I like the fact that it calls setRunFolder() which may be something that is missing on my end.

Comment 19

[:aceman]

(In reply to Axel Grude [:realRaven] from comment #18)
> I set the feedback from ? to +  via Patch > Details... I hope I did this
> right, I couldn't find any instructions on how to approve it. 

Yes, that is correct. You can also do that via the "Splinter Review" link.
Thanks for trying it out.

Comment 20

[:aceman]

Attachment: patch v1.1

Fixes the problem alta has found.

Comment 21

[Jorg K (CEST = GMT+2)]

Hmm, review requested one year ago :-(

Aceman, does the patch still apply? Maybe find a different reviewer.

Comment 22

[:aceman]

Attachment: patch v1.2

Thanks for the ping. Yeah, sadly no review. This is a refreshed patch.
Can you try to review it based on the fact that Alta and RealRaven have played with it?

Comment 23

[Jorg K (CEST = GMT+2)]

I inherited this review. Aceman, could you "review" your own patch and add comments that motivate what's going on. Thanks in advance.

Comment 24

[:aceman]

The description of the changes is in comment 8. How can I review my own patch? :)

Comment 25

[Jorg K (CEST = GMT+2)]

You open the review, and then associate the comments from comment #8 with the respective lines. That makes it easier to understand the individual changes.

Comment 26

[:aceman]

Comment on attachment 8957853 [details] [diff] [review]
patch v1.2

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

::: mail/base/content/FilterListDialog.js
@@ -129,5 @@
>   */
>  function processWindowArguments(aArguments) {
> -  // If a specific folder was requested, try to select it.
> -  if (!gSelectedFolder ||
> -      (("folder" in aArguments) && (aArguments.folder != gCurrentFolder)))

This was the point of failure. This didn't evaluate correctly (possibly the g*Folder values weren't set properly), thus it didn't take the branch to rebuildFilterList() to redraw the list and make the new filter visible.
What is the difference between gSelectedFolder and gCurrentFolder? They look the same so I removed them both. When we need to see which folder/server is selected (and we manage the filters for), we can just get that from gServerMenu._folder .

@@ -185,5 @@
> - * better named 'onServerSelect' => file follow up bug later.
> - *
> - * @param aFolder  the nsIMsgFolder that was selected
> - */
> -function onFilterFolderClick(aFolder)

Merged into setFolder(), creating setFilterFolder().

@@ -276,5 @@
>    aFilterItem.setAttribute("aria-checked", filter.enabled);
>  }
>  
> -// update the server menulist
> -function selectFolder(aFolder)

Merged into setFilterFolder().

@@ -915,5 @@
>      }
>    }
>  }
>  
> -function onTargetSelect(event) {

Inlined this into the xul file.

@@ -919,5 @@
> -function onTargetSelect(event) {
> -  setRunFolder(event.target._folder);
> -}
> -
> -function setRunFolder(aFolder) {

Just moved elsewhere.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957853 [details] [diff] [review]
patch v1.2

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

I haven't tried it, but I think we could straighten out the issues below first.

::: mail/base/content/FilterListDialog.js
@@ +205,2 @@
>  
> +  // Setting this attribute should go away in bug 473009.

Ah, insider knowledge here.

@@ +205,4 @@
>  
> +  // Setting this attribute should go away in bug 473009.
> +  gServerMenu._folder = msgFolder;
> +  // Calling this should go away in bug 802609.

And here.

@@ +208,5 @@
> +  // Calling this should go away in bug 802609.
> +  gServerMenu.menupopup.selectFolder(msgFolder);
> +
> +  // Calling getFilterList will detect any errors in msgFilterRules.dat,
> +  // backup the file, and alert the user.

Interesting comment, but where is getFilterList() called? I cannot see it.

@@ +237,5 @@
>     document.getElementById("folderPickerPrefix").hidden = !canFilterAfterTheFact;
>  
> +  if (canFilterAfterTheFact) {
> +    // For NNTP select the subscribed newsgroup. For others use the root server.
> +    let wantedFolder = (msgFolder.server.type == "nntp" ? gServerMenu._folder : msgFolder);

Well, getFirstFolder() checks for isServer, so msgFolder.server may be undefined and this new check may fail.

@@ +240,5 @@
> +    // For NNTP select the subscribed newsgroup. For others use the root server.
> +    let wantedFolder = (msgFolder.server.type == "nntp" ? gServerMenu._folder : msgFolder);
> +
> +    // Select a useful first folder for the server.
> +    setRunFolder(getFirstFolder(wantedFolder));

Any chance to clean up this mess while were here: Eliminate getFirstFolder() and inline the code here.

Mostly it looks at msgFolder.server.type. You've got nntp processing here and in the function and it's all unclear.

How about:
if (!msgFolder.isServer) {
  wantedFolder = msgFolder;
} else {
  switch (msgFolder.server.type) {
  case "nntp":
    wantedFolder = gServerMenu._folder;
    break;
  case "rss":
    wantedFolder = null;
    break;
  case "imap":
  case "pop":
    wantedFolder = msgFolder.rootFolder.getFolderWithFlags(Ci.nsMsgFolderFlags.Inbox);
    break;
  default:
    // ??
  }
}
setRunFolder(wantedFolder);

Is there more to it? What am I missing?

Comment 28

[:aceman]

(In reply to Jorg K (GMT+1) from comment #27)
> @@ +237,5 @@
> >     document.getElementById("folderPickerPrefix").hidden = !canFilterAfterTheFact;
> >  
> > +  if (canFilterAfterTheFact) {
> > +    // For NNTP select the subscribed newsgroup. For others use the root server.
> > +    let wantedFolder = (msgFolder.server.type == "nntp" ? gServerMenu._folder : msgFolder);
> 
> Well, getFirstFolder() checks for isServer, so msgFolder.server may be
> undefined and this new check may fail.

I'm not sure what you mean here. msgFolder does not need to be a server, but msgFolder.server will be. Unless msgFolder is undefined. Inside getFirstFolder(), we also read msgFolder.server.type .

> @@ +240,5 @@
> > +    // For NNTP select the subscribed newsgroup. For others use the root server.
> > +    let wantedFolder = (msgFolder.server.type == "nntp" ? gServerMenu._folder : msgFolder);
> > +
> > +    // Select a useful first folder for the server.
> > +    setRunFolder(getFirstFolder(wantedFolder));
> 
> Any chance to clean up this mess while were here: Eliminate getFirstFolder()
> and inline the code here.
> 
> Mostly it looks at msgFolder.server.type. You've got nntp processing here
> and in the function and it's all unclear.

Yes, these can be folded when this is the only caller of getFirstFolder().

Comment 29

[:aceman]

Attachment: patch v1.3

Thanks, so this should be the cleanup.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971836 [details] [diff] [review]
patch v1.3

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

Needs a bit of clean-up, but let me try it first.

::: mail/base/content/FilterListDialog.js
@@ +237,5 @@
>     document.getElementById("folderPickerPrefix").hidden = !canFilterAfterTheFact;
>  
> +  if (canFilterAfterTheFact) {
> +    let wantedFolder = null;
> +Cu.reportError(msgFolder.name);

What's that doing here?

@@ +266,5 @@
> +          // so show "Choose Folder".
> +          wantedFolder = null;
> +        }
> +      } catch (e) {
> +        dump(e + "\n");

The dump() doesn't really help us. Make that something else, please.

@@ +270,5 @@
> +        dump(e + "\n");
> +        wantedFolder = null;
> +      }
> +    }
> +Cu.reportError(wantedFolder ? wantedFolder.name : null);

What's that doing here?

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971836 [details] [diff] [review]
patch v1.3

Works for me, thanks. Please remove the reportError and replace the dump().

Comment 32

[:aceman]

Attachment: 1328855.patch v1.4

Thanks, fixed the debug output.

Comment 33

[Jorg K (CEST = GMT+2)]

Can we shorten the commit message a bit:
"clean up tracking of which folder is selected in which picker in FilterListDialog.js and make refresh() redraw the filter list again."

Where is that refresh happening?

How about: "Clean up folder handling in FilterListDialog.js"?

Comment 34

[:aceman]

Sure, you can shorten the message.
Refresh() is the function that calls processWindowArguments() that is being fixed in this patch.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1d7d01506fe6
Clean up folder handling in FilterListDialog.js and fix filter list redraw. r=jorgk

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973565 [details] [diff] [review]
1328855.patch v1.4

Maybe not for beta, but certainly a candidate for ESR 60.

Comment 37

[Jorg K (CEST = GMT+2)]

Beta (TB 60):
https://hg.mozilla.org/releases/comm-beta/rev/6510ef9822b357451a319269896557211585845f