Message filter not added to open "Message Filter" dialogue

RESOLVED FIXED in Thunderbird 61.0

Status

defect
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: dutch_gemini, Assigned: aceman)

Tracking

({regression})

45 Branch
Thunderbird 61.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird60 fixed, thunderbird61 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

2 years ago
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.
Assignee

Comment 1

2 years ago
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

2 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 3

2 years ago
Nothing in error console.

Updated

2 years ago
Duplicate of this bug: 1340074
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.
Assignee

Comment 6

2 years ago
I do see it now.
Assignee: nobody → acelists
OS: Unspecified → All
Hardware: Unspecified → All
Assignee

Comment 7

2 years ago
I think this got regressed by bug 1025373 or bug 363982.
Status: NEW → ASSIGNED
Keywords: regression
Assignee

Comment 8

2 years ago
Posted patch patch (obsolete) — Splinter Review
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?
Attachment #8841799 - Flags: review?(mkmelin+mozilla)
Attachment #8841799 - Flags: feedback?(alta88)
Assignee

Comment 9

2 years ago
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.
Attachment #8841799 - Flags: feedback?(axelg)

Comment 10

2 years ago
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.
Attachment #8841799 - Flags: feedback?(alta88) → feedback+

Comment 11

2 years ago
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.
(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);
    
  }
Assignee

Comment 13

2 years ago
(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.
Assignee

Comment 14

2 years ago
(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.
(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?
(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...
Assignee

Comment 17

2 years ago
(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.
Attachment #8841799 - Flags: feedback?(axelg) → feedback+
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.
Assignee

Comment 19

2 years ago
(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.
Assignee

Comment 20

2 years ago
Posted patch patch v1.1 (obsolete) — Splinter Review
Fixes the problem alta has found.
Attachment #8841799 - Attachment is obsolete: true
Attachment #8841799 - Flags: review?(mkmelin+mozilla)
Attachment #8846266 - Flags: review?(mkmelin+mozilla)

Comment 21

a year ago
Hmm, review requested one year ago :-(

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

Comment 22

a year ago
Posted patch patch v1.2 (obsolete) — Splinter Review
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?
Attachment #8846266 - Attachment is obsolete: true
Attachment #8846266 - Flags: review?(mkmelin+mozilla)
Attachment #8957853 - Flags: review?(jorgk)

Comment 23

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

Comment 24

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

Comment 25

a year ago
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.
Assignee

Comment 26

a year ago
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

a year ago
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?
Attachment #8957853 - Flags: review?(jorgk)
Assignee

Comment 28

a year ago
(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().
Assignee

Comment 29

a year ago
Posted patch patch v1.3 (obsolete) — Splinter Review
Thanks, so this should be the cleanup.
Attachment #8957853 - Attachment is obsolete: true
Attachment #8971836 - Flags: review?(jorgk)

Comment 30

a year ago
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

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

Works for me, thanks. Please remove the reportError and replace the dump().
Attachment #8971836 - Flags: review?(jorgk) → review+
Assignee

Comment 32

a year ago
Thanks, fixed the debug output.
Attachment #8971836 - Attachment is obsolete: true
Attachment #8973565 - Flags: review+

Comment 33

a year ago
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"?
Assignee

Comment 34

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

Comment 35

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1d7d01506fe6
Clean up folder handling in FilterListDialog.js and fix filter list redraw. r=jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Updated

a year ago
Target Milestone: --- → Thunderbird 61.0

Comment 36

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

Maybe not for beta, but certainly a candidate for ESR 60.
Attachment #8973565 - Flags: approval-comm-beta+
Assignee

Updated

7 months ago
Duplicate of this bug: 397737
You need to log in before you can comment on or make changes to this bug.