42090 - Filter UI: cancel after editing filters keeps changes

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect, P3)

Description

[laurel]

Using jun09 m17 commercial build

1.  From mail window, launch message filters ui for an account which already has
several filters.
2.  Select a filter in the list, move it up or downward using arrow widgets.
3.  Click Cancel button.
4.  Launch message filters ui again.

Result:   Change in filter order was preserved even though you Canceled from the
dialog.

Comment 1

[Alec Flett]

I don't ever expect to fix this.

Comment 2

[Alec Flett]

reassigning my filter bugs to gayatrib..

Comment 3

[Håkan Waara]

This bug would be INVALID if we go with the "'Close' instead of 'Cancel' for
button name" solution. Otherwise this will rot yet another year...

Comment 4

[Matthew T (active 1999-2002)]

No button in a window should ever be labelled `Close'. If changes in a window 
are live, then it shouldn't have `Cancel' and `OK' buttons at all.

Comment 5

[jglick]

So just remove the "Cancel" and "OK" buttons completely from the dialog?

Comment 6

[Matthew T (active 1999-2002)]

Attachment: Yes, like this.

Comment 7

[Blake Ross]

I agree that dialogs should never have a Close button.  I'm always scared to 
press them because I have no idea if my changes are being saved or not.

However, I am often equally confused about what closing the dialog with the 
native close widget will do (save or not save).  I also fear that I don't 
recall seeing [m]any dialogs on Windows without buttons (although I won't claim 
they don't exist, for fear of being presented with a long list of them...)

That said, I'm not sure I see why this dialog shouldn't have a Cancel button 
(aside from `I don't ever plan on implementing this').  It's not critical, 
because it has the (more useful) Remove button, but I don't think it could 
hurt.  If one did exist, I would expect it to revert any ordering changes I 
made since I opened the dialog.

Comment 8

[jglick]

See bug 40512. Creating Filters is a dialog (Message Filters) that opens a 
dialog (Filter Rules), so its not clear if canceling the parent dialog cancels 
only the changes made on that parent dialog, or if it also cancels any changes 
that were made on the child dialog too (if the child dialog was OK'd).  Some say 
its a given that canceling the parent dialog should also cancel any changes made 
to the child dialog. This might not be obvious to users, and MS can't even 
consistently decide what should happen in their own UI's. Using "Close" instead 
would eliminate the confusion.

Currently, both "OK" and "Cancel" on the Message Filters dialog is behaving like 
"Close". I would suggest going with mpt's suggestion of removing the buttons to 
reflect the current behavior (or changing to a "Close" button).

I say 'or changing to a Close button' because the "Seach Mail/News" dialog has a 
"Close" button.  Not that is the correction solution, but that we should decide 
which way to go (with "Close" or with no buttons) and be consistent.

Comment 9

[Blake Ross]

Ok, then when given a choice between a Close button and no button(s) at all, 
I'd choose the latter.

It still makes me wonder why we're in this predicament if other apps aren't.  
Can someone point out some Windows apps that do this so I feel better?

Comment 10

[Matthew T (active 1999-2002)]

Blake, we're in this predicament, whereas other apps aren't, *only* because of 
the `I don't ever expect to fix this' thing. If someone could fix it, we 
wouldn't have this problem.

Here's a brief description of the two types of window involved here.

Dialog                                 Document/utility window
------                                 -----------------------
*   Modal to the parent window         *   Non-modal
*   Cannot be minimized independently  *   Can be minimized
    of the parent window
*   Closed using any one of a row of   *   closed using a close box in the
    buttons (e.g. `Cancel', `OK')          window chrome
*   does not have menus (except for    *   may have menus
    `Edit' on Mac OS)
*   changes are not applied until the  *   changes are ideally live; failing
    dialog is dismissed                    that, they are applied when the
                                           window is closed or the user chooses
                                           `File' > `Save'
*   Examples:                          *   Examples:
    -   Mozilla Preferences                -   Finder Preferences    
    -   formatting dialogs                 -   Windows Calculator
    -   Mozilla Account Manager            -   WordPad

Now, there are a lot of windows in various apps which blur the distinction 
between these two types -- and indeed, the UI guidelines for both Windows and 
Mac OS allow for some level of blurring -- but doing so invariably causes UI 
problems. (What does it mean if I click the close box in the `You are about to 
migrate your 4.x profile' window in Mozilla? Does it make a new profile, or not 
start Mozilla at all? What happens if I click `Cancel' in the Add/Remove 
Programs control panel in Windows 9x? Does it reinstall programs which I've 
just uninstalled?) It's a shame that XP Toolkit even allows such half-bred 
contraptions in the first place.

The Filters window is another such awkward example: it looks like a dialog, but 
it is non-modal like a document/utility window, and -- as this bug and bug 
40512 explain -- changes made in it are live, rather than being applied when 
the user clicks `OK'. So we have two possibilities for turning it into decent 
UI:
(1) find someone to fix the bugs to allow the Filters window to be a properly
    functioning dialog -- make it modal, and don't apply changes until and
    unless `OK' is clicked;
(2) turn the Filters window into a properly-designed document/utility window
    -- make it closable and minimizable, and remove the `OK' and `Cancel'
    buttons.

I think that of these two options, (2) would result in greatest usability; 
people who use mail filters are likely to be able to handle the modelessness 
anyway, and indeed they may need it in order to switch back to the three-pane 
and browse through messages which they want to make a filter for. 
Coincidentally, (2) would also involve the least amount of work. :-)

Comment 11

[Blake Ross]

But neither (1) nor (2) are going to happen if this remains assigned to an 
engineer who's no longer here.  Reassigning.

Comment 12

[Simon Fraser [no longer active]]

*** Bug 93967 has been marked as a duplicate of this bug. ***

Comment 13

[Simon Fraser [no longer active]]

Is it so hard to have the dialog act on a copy of the filter data in memory, and 
only commit that if the user confirms the dialog?

I think this is a P2 bug. I've trashed my filters because of it.

Comment 14

[Navin Gupta]

over to seth

Comment 15

[Jim Song]

Hi, Is it possible to make an temporary file to copy all the filter data
out before editing? ( I see nsMsgFilterList::SaveToDefaultFile etc. functions )

If have changes and cancel, then restore this file?

Comment 16

[Jim Song]

Hi, 
I saw that the cancel button really WORKs at a certain extent.
If you click the OK button, the MsgFilterService will write the data 
to the disk; But if you click the CANCEL button, the MsgFilterService 
will not write out the data. What we see after we click the CANCEL is 
only the in-memory data.

So, when the user click the CANCEL, we can refresh the memory data 
immediately to clean this bug, right?

Comment 17

[(not reading, please use [e-mail] instead)]

*** Bug 40512 has been marked as a duplicate of this bug. ***

Comment 18

[(not reading, please use [e-mail] instead)]

I think jim song is on to how to fix this.

when the user hits cancel, we don't appear to write out the changes to disk, so 
the changes are only made "in memory".

we might be able to do as he suggests, and on cancel, reset things to how they 
were from the file.

before any work begins, I'll want to check with jglick.  

if she wants to change the filters spec and not have ok / cancel buttons, there 
would be no point in doing this work.

setting to 1.0.1 for now, it might move in depending on what I found out from 
jglick.

Comment 19

[Jim Song]

Attachment: FilterListDialog.js.diff

Change in FilterListDialog.js

Comment 20

[Jim Song]

Attachment: nsIMsgFilterList.idl.diff

Change in nsIMsgFilterList.idl
( Add the new method)

Comment 21

[Jim Song]

Attachment: nsMsgFilterList.cpp

Change in nsMsgFilterList.cpp
( implement of the new function )

Comment 22

[Jim Song]

Hi, Seth, 
I have posted three patches for discussion. They DO
cancel your changes, but I am not sure about the safety
problem when Releasing the filterlist array.

Comment 23

[Håkan Waara]

Jim, nsFileSpec is a dead ghost that people are trying to remove - please do not
add new references to it. There's a better replacement (nsIFile?)...

Seth, when I redesigned this dialog, I made it per the spec, which included the
OK/Cancel buttons. I don't see a point of removing them, since you really want a
way to cancel changes in dialogs like this one.

Seth, can we bring this back since it has a patch?

Comment 24

[jglick]

Question still remains:

User opens the Message Filter dialog. 
Clicks "new" and opens the Filter Rules dialog. 
Enters valid info. 
Hits ok to close Filter Rules.
Clicks Cancel on Message Filter dialog.

Does the new filter just created remain or get canceled? I would think the new 
filter just created is canceled as well. Is this part of the proposed fix?

Comment 25

[Håkan Waara]

I agree with Jglick here; after all, that's what the "Cancel" button is there
for in the first place. :-)

Comment 26

[Jim Song]

Jglick,
Yes, of couse, It is CANCELLED.
If there is no safety problem about the releasing procedure, I could provide
standard patches.

Comment 27

[jglick]

>Yes, of couse, It is CANCELLED.
Great. Thanks for the help with this.

Comment 28

[Jim Song]

Attachment: Final patches( to reload)

Please review this patch. (obsolete the old ones)

Comment 29

[Margaret Chan]

Is the patch good to go?  Please review.

Comment 30

[Margaret Chan]

Can someone review the patch please?  Margaret

Comment 31

[Håkan Waara]

Comment on attachment 68516 [details] [diff] [review]
Final patches( to reload)

Firstly, please avoid all tabs and replace them with the same indentation as
the rest of the file uses -- with spaces only.

>+    if (filterList) filterList.reloadTextFilters();

Please put the if (...) and the function call on separate lines.

>+    void reloadTextFilters();

Does "text" filters imply that there are other non-text filters? Maybe you
should change the name to reloadFilters().

>+	//Clean the Array

Since m_filters is a nsCOMPtr (nsISupportsArray), I think you can safely call
clear() on it, and all its items will be freed for you. If I am right, you can
remove the whole for-loop.

>+
>+	//Get File Stream
>+	nsFileSpec filterSpec;
>+	m_defaultFile->GetFileSpec(&filterSpec);

I suggest you make that a nsCOMPtr<nsIFileSpec> -- it looks like you are
leaking the filespec? Anyway, the good thing about comptrs is that you don't
need to worry (as much) about leaks. :-)

Comment 32

[Håkan Waara]

BTW, please obsolete the old patches using the patch-manager the next time you
attach a new patch (to avoid confusion about which to review).  Thanks!

Comment 33

[Jim Song]

Attachment: ro reload filters when Cancel

Hi, Waara,
Thank you very much for reviewing.
I have offered the new patch according to your suggestion.

1)Change reloadTextFilters to reloadFilters. I used to use the previous 
  name because the class have another function called loadTextFilters.
2)Change the filterSpec to nsCOMPtr.
3)Change tabs to spaces. I used the tabs since most part of the file use
   it instead of spaces.

But I have not change the loop to clean the filters, since I am not sure
the array could release the memory pointered by each item. Anyway, it works
now.

Please review the new patch again. :)

Comment 34

[Jim Song]

Hi, Waara,
I am sorry, I have no rights to obsolete my old patches! :(

Comment 35

[Håkan Waara]

Comment on attachment 72389 [details] [diff] [review]
ro reload filters when Cancel

(Sorry for the spam, all.)

>+  //Clean the Array
>+  prUint32  filterCount;

This will break most compilers. Should be 'PRUInt32', if I recall correctly.

>+  for (PRUint32 i = 0; i < filterCount; i++)

I think you'll need to define the integer outside the for-loop, since some
compilers will choke on this. For further information about coding-standards
and similar, see the "C++ portable guidelines" on mozilla.org -- it's must for
every mozilla developer.

>+  m_filters->Clear();
>+
>+  //Get File Stream
>+  nsCOMPtr<nsFileSpec> filterSpec;
>+  m_defaultFile->GetFileSpec( getter_AddRefs(filterSpec) );
>+  if( !filterSpec )
>+    return NS_ERROR_FAILURE ;
>+
>+  //Reopen the File
>+  nsIOFileStream *fileStream = new nsIOFileStream(filterSpec);

You never |delete| this pointer. Is it leaking, or am I missing something?

r=hwaara if you fix these issues or explain why you don't need to. :-)

Comment 36

[Jim Song]

Attachment: Cleaner Patch

Hi, Hakan,
I have done some another cleaner work:
1) change prUint32 to PRUnit32
2) move definitionof i out of the loop
3) delete the stream. :(  (I copy it from some other place, thought the close
will delete itself...)

But I change the nsCOMPtr back to object, because we already have the interface

m_defaultFile, and have to get an object to carter constructor of
nsIOFileStream.

Pls review again. Thanks. :)
BTW, will my patched be added to 1.0?

Comment 37

[Håkan Waara]

Comment on attachment 72556 [details] [diff] [review]
Cleaner Patch

If you are gonna change it back to a raw interface-pointer, you need to free
the memory you allocate, right?  I am not sure you do that in this patch.

for ( i = 0; i < filterCount; i++)

Can you please remove the initial space in that parentheses?  Also, try to be
consistent in other places where you insert spaces.

Comment 38

[Jim Song]

Hi, Warra,
I assure you the object can clean itself when
it is removed from the stack. I check all 
usages of this object in other files, I do 
the same work with them.

As to the space, I think it will make the code
more legible. If you also believe so, I would
like to add more spaces. What do you suggest or 
just leave them there?

Comment 39

[Håkan Waara]

Jim, please use the same coding style as the rest of the file already does, so
everything will look consistent.

Comment 40

[Håkan Waara]

Jim, I just confirmed that you don't need that for-loop.  In fact, it's totally
redundant.  You will get all items as new pointers, and free them one by one,
then when you call .clear() on the array only then the items will be removed and
freed properly. So, you just want to .clear() the array -- remove the iteration.

Fix that and make the indentation + spacing consistent with the rest of the
file, and I think we can finally pass this onto sr=.  :-)

Comment 41

[Jim Song]

Attachment: patch

Hi, Waara,
New Patch. :)
I have remove the loop, and the spaces.
Hmm, seems cleaner.
The indent is 2 spaces as Mozilla required, 
should I change it back to the tabs?

Comment 42

[Håkan Waara]

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

Will you need to release/free that nsFileSpec?

r=hwaara

Comment 43

[David :Bienvenu]

couldn't we just cause rdf to reload the filter list from the file again,without
having a new method in the filter list?

Comment 44

[Jim Song]

Hi, Bienvenu,
Could you upload you patch or tell me how to do it?

Comment 45

[Joshua Xia]

Hi, Scott, Spitzer

Please super-review this patch,

thanks!

Comment 46

[David :Bienvenu]

I don't have a patch to cause rdf to reload the filter list, but I'd be
surprised if there wasn't a way, and that would be a much better way to fix this
than the proposed patches. Even better would be to simply not write out the
filter file at all when cancel is pressed.

Comment 47

[David :Bienvenu]

my feeling is that the correct way to fix this is to add a cancel handler for
the dialog (there isn't one now!), and in it, to clear the filterList for any
servers whose filters we've edited, something like:

+function onCancel()
+{
+ // for each server we've edited, clear the filterList in memory like this:
//   server.filterList = nsnull;
+  window.close();
+}
 function onServerClick(event)
 {
     var item = event.target;

and in 
 FilterListDialog.xul
   buttons="accept,cancel,help"
   onload="onLoad();"
   ondialogaccept="return onOk();"
+  ondialogcancel="return onCancel();"
   ondialoghelp="return doHelpButton();"
   width="440"
   height="320"

if we do this, we'll be clearing out the filter list in memory that's been
edited, which will force us to reload it from disk.

Comment 48

[Joshua Xia]

Attachment: This is the final patch

This patch forces filter list to be reload if the "Cancel" button is clicked,
it works well.

Please review or super-review it ASAP.

Thanks!

Comment 49

[David :Bienvenu]

I'll say this one more time - I don't want a new method added to the filter list
to reload filters.

Comment 50

[Joshua Xia]

Sorry, I haven't found a way to set "server.filterList = nsnull" to clear
current filter list to force to reload list from disk automatically.

Comment 51

[David :Bienvenu]

what do mean, you couldn't find a way to clear the server filterlist? there's a
method setFilterList on nsIMsgIncomingServer - could you not find that, or did
it not work?

Comment 52

[Joshua Xia]

Sorry, There is no "setFilterList" or "SetFilterList" in nsIMsgIncomingServer,
There is only "GetFilterList" in nsIMsgIncomingServer.

Comment 53

[Joshua Xia]

My source code is too old, I will try new version.

Comment 54

[Joshua Xia]

I add the following source code:

function onCancel()
{
  var firstItem = getSelectedServerForFilters();
  if (!firstItem)
    firstItem = getServerThatCanHaveFilters();

    if (firstItem) {
        var resource = rdf.GetResource(firstItem);
        var msgFolder =  resource.QueryInterface 
(Components.interfaces.nsIMsgFolder);
        if (msgFolder)
            msgFolder.setFilterList(null);
   }

  window.close();
}
in FilterListDialog.js

and 

NS_IMETHODIMP
nsMsgFolder::SetFilterList(nsIMsgFilterList *aFilterList)
{
  nsresult rv;
  nsCOMPtr<nsIMsgIncomingServer> server;
  rv = GetServer(getter_AddRefs(server));
  NS_ENSURE_SUCCESS(rv, rv);
  NS_ENSURE_TRUE(server, NS_ERROR_FAILURE);

  return server->SetFilterList(aFilterList);
}
in nsMsgFolder.cpp

so the filter list is set to be null when click "Cancel" button (as Bienvenu 's 
suggestion), but it don't work. the filter list's order isn't reloaded 
automatically. We must pay attention that the order of filter list is save into 
m_filters in nsMsgFilterList.cpp.

Comment 55

[Joshua Xia]

I tried another way to fix this bug: in nsMsgFilter.cpp, save m_filters to a 
m_tmpfilters, fresh m_tmpfilters's order when "OK" button is clicked, and 
refresh m_filters 's order from m_tmpfilters if "Cancel" button is clicked, 
this method needn't reload filters.

I tried the following code, it work well:

NS_IMETHODIMP
nsMsgFilterList::SaveToDefaultFile()
{
    nsresult rv;
    nsCOMPtr<nsIMsgFilterService> filterService =
        do_GetService(kMsgFilterServiceCID, &rv);
    NS_ENSURE_SUCCESS(rv, rv);
    
    // clone m_filters to m_oldfilters
    m_oldfilters->Clear();
    
    PRUint32  filterCount;
    m_filters->Count(&filterCount);
    
    for (PRUint32 i = 0; i < filterCount; i++)
	{
        m_oldfilters->AppendElement(m_filters->ElementAt(i));
    }
    
    return filterService->SaveFilterList(this, m_defaultFile);
}

NS_IMETHODIMP
nsMsgFilterList::ReloadFilterListOrder()
{
    // clone m_oldfilters to m_filters
    PRUint32  filterCount;
    m_oldfilters->Count(&filterCount);
    
    m_filters->Clear();
        
    for (PRUint32 i = 0; i < filterCount; i++)
    {
       m_filters->AppendElement(m_oldfilters->ElementAt(i));
    }
    return NS_OK;
}

Comment 56

[David :Bienvenu]

your previous (2002-03-06 18:46) patch was almost correct. If you change
SetFilterList as follows, it should work for you. 

NS_IMETHODIMP
nsMsgFolder::SetFilterList(nsIMsgFilterList *aFilterList)
{
  nsresult rv;
  nsCOMPtr<nsIMsgIncomingServer> server;
  rv = GetServer(getter_AddRefs(server));
  NS_ENSURE_SUCCESS(rv, rv);
  NS_ENSURE_TRUE(server, NS_ERROR_FAILURE);

  ReleaseDelegate("filter"); // make RDF forget about delegate
  return server->SetFilterList(aFilterList);
}

Comment 57

[David :Bienvenu]

actually, not the patch you attached, but the patch you put in a comment dated
2002-07-16 04:44, was the one that was one line away from working. There is one
other issue, which is that you should probably override SetFilterList in
nsImapMailFolder to assign the m_filterList member variable and then call the
base class implementation of SetFilterList.

Comment 58

[Joshua Xia]

Attachment: New patch that needn't reloadFilterList method

This is the patch that needn't reloadFIlterList method (as Bienvenu 's
suggestion). this patch set filterList to be null when "Cancel" button is
clicked and force filter order to be reloaded automatically when the dialog
reopen. Thank Bienvenu very much for his help.

Please review this path ASAP
Thanks!

Comment 59

[David :Bienvenu]

You also need to override SetFilterList in nsImapMailFolder to look like this:

NS_IMETHODIMP nsImapMailFolder::SetFilterList(nsIMsgFilterList *aMsgFilterList)
{
  m_filterList = aMsgFilterList;
  return nsMsgFolder::SetFilterList(aMsgFilterList);
}

otherwise, the imap folder will hold onto the out of order filter list and
filters will fire in the wrong order until you shutdown and restart.

Comment 60

[Joshua Xia]

Attachment: modified patch

Append override function into patch 91589, it works well.
Thank Bienvenu's help!

Comment 61

[David :Bienvenu]

Joshua, can you get a review from, say, Navin, and I'll do the sr? thx.

Comment 62

[Joshua Xia]

You means Naving? His email [e-mail]?
OK, I send review form to him. Thanks!

Comment 63

[Navin Gupta]

Comment on attachment 91744 [details] [diff] [review]
modified patch

This patch is not correct because you should get inbox folder
and set filter list for 
inbox to null. we only
get filterlist for imap inbox. For local we always get it from server.

Comment 64

[Joshua Xia]

So. I need only to override "SetFilterList" in nsLocalMailFolder.cpp, right?
Thanks!

Comment 65

[Joshua Xia]

For local Folder, How can we create filter list?

Comment 66

[Navin Gupta]

Set filterlist null for imap Inbox. So when you are getting folder here get inbox 
folder in case of imap. 

f (firstItem) {
+        var resource = rdf.GetResource(firstItem);
+        var msgFolder =
resource.QueryInterface(Components.interfaces.nsIMsgFolder);
+        if (msgFolder)
+            msgFolder.setFilterList(null);
+    }

Comment 67

[David :Bienvenu]

Navin, the imap case works fine as is. Joshua and I both thought you were
pointing out a problem in the local/pop3 case. Could you clarify?

Comment 68

[Navin Gupta]

How does the imap case work fine as is ? On the contrary local case is fine as
is (modified patch). For imap we need to set filter list null for inbox. 

we cache the filterList for imap inbox here in nsImapMailFolder::UpdateFolder
line 626 

  if (mFlags & MSG_FOLDER_FLAG_INBOX && !m_filterList)
    rv = GetFilterList(msgWindow, getter_AddRefs(m_filterList));

Comment 69

[David :Bienvenu]

Ah, I think I understand what you were saying, Navin, thanks. You're right, but
IFF I understand your suggestion (call setFilterList on the inbox for imap, the
root folder for pop3), it's not quite sufficient.

The patch as is does work for the imap case, in the sense that the filter ui
shows the correct filter ordering afer cancel and reload. This is because the
filter ui gets the filter list from the incoming server (via the root folder for
the server, which is not the INBOX, but the folder with the same name as the
server). This is true for both imap and pop3. The problem is that the
nsImapMailFolder for the INBOX caches the filter list from the incoming
server/root folder, and this is the filter list used when actually executing
filters on the INBOX. Put another way, both pop3 and imap get the filter list
from the same place (the root folder for the server), but the imap INBOX caches
this filter list. 

So, to fix the order for filter execution in the imap case, we need to make sure
that we call setFilterList on the imap INBOX so that the imap INBOX can clear
its cached root folder filter list. But we still need to make sure that the root
folder for the imap server gets called with setFilterList(nsnull) because that's
what the filter UI uses. So, for imap, we need to call setFilterList on both the
inbox and the root folder. To get the inbox, you need to take the rootMsgFolder,
and write some code like this:
       //now find Inbox
        var outNumFolders = new Object();
        var inboxFolder = rootMsgFolder.getFoldersWithFlag(0x1000, 1,
outNumFolders);

        return inboxFolder.QueryInterface(Components.interfaces.nsIMsgFolder);

Since this code is in a few places, we might want to move it to a common place,
perhaps "widgetglue.js", and included that in the filterListDialog.xul.

For pop3, it doesn't hurt to call setFilterList on both the inbox and the root
folder, so you don't really need to check for imap/pop3.

The other alternative for fixing this, which I probably should have suggested
from the start, is have the filter editor code edit a *copy* of the filter list,
and only on OK, do we update the actual filter list (this copy should be
maintained by the filter list editing js code, not by the backend nsMsgFilterList).

Comment 70

[Navin Gupta]

Who uses the filterList on the rootFolder? can you point me there. It should be
getting from server.

Comment 71

[Navin Gupta]

Only for imap folder we cache the filter list and we don't do this for any other
folder type (neither base or local). and in ParseMailbox (local) we are
getting filterList from server, so please clarify?

Comment 72

[David :Bienvenu]

I can't point you to a particular piece of code, since it's the magic filter
editor rdf datasource code, which I believe, like me, you've also had the
pleasure of banging your head against :-)

Comment 73

[David :Bienvenu]

Navin, if you go look at http://bugzilla.mozilla.org/show_bug.cgi?id=42090#c56,
you'll see the call to ReleaseDelegate("filter") - this is because there's an
rdf delegate in the nsMsgFolder holding onto the filter list. I'd love to
explain more, but I don't really know what the heck an rdf delegate is, except
that it's something you can use to hold onto a filter list :-)

Comment 74

[Navin Gupta]

ok, thanks. I checked it we are doing that filterList delegate on root folder. 

so in the patch we should make it

if(mIsServer)
  ReleaseDelegate("filter")

Comment 75

[David :Bienvenu]

As long as Release'ing a delegate that doesn't exist isn't a problem, I don't
think that check is needed or neccesarily a good idea - at some point, we'll
have filters on newsgroups and public imap folders, and perhaps folders in
general, and that check will cause problems then.

Comment 76

[Navin Gupta]

If you look into 

nsRDFResource::ReleaseDelegate(const char* aKey) it warns you

NS_WARNING("nsRDFResource::ReleaseDelegate() no delegate found");

if there is no delegate for that key, which is "filter" here. 

Comment 77

[David :Bienvenu]

that doesn't seem like a big deal - it just dumps a message on the console,
right?  It doesn't cause assertions or errors? Unfortunately, I don't see any
way of finding out if a resource has a delegate or not.

Comment 78

[David :Bienvenu]

we might move the ReleaseDelegate call to the js code, since it's the filter
UI/DS that knows it created a delegate in the first place.

Comment 79

[Joshua Xia]

If it is difficult to make a ideal patch for this bug. Can we checkin the
2002-03-06 's patch (which has been reviewed) into OEM branch? because this
patch has been check into 6.2.2, so we don't want to make this bug regression.

Thanks!

Comment 80

[Navin Gupta]

ok, I'll attach a patch as per what david and I discussed. 

Comment 81

[Navin Gupta]

Attachment: patch

Made changes to last patch by setting inbox folder filter list to null. moved 
ReleaseDelegate call to js and I'm not including widgetglue because we need
just
3 lines for inbox here. David, can you sr ? thx. I have tested the fix for
imap.

Comment 82

[David :Bienvenu]

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

sr=bienvenu - I realize it's just three lines of js, but those three lines
occur in 4 places in the js code, and they have that hardcoded flag in each of
them - that's why I wanted it in a common place (it's also a handy routine :-)
) but it's OK as is.

Comment 83

[Navin Gupta]

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

r=naving

Comment 84

[Navin Gupta]

We can use var MSG_FOLDER_FLAG_INBOX = 0x1000, that will probably be easier to
catch. 

I have asked trunk-approval for that patch, will check it in as soon as I get it. 

Comment 85

[Joshua Xia]

Navin and Bienvenu:

Thanks a lot!

Comment 86

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.1

Comment 87

[Navin Gupta]

fix checked on trunk.

Comment 88

[Bolian Yin]

checked in NETSCAPE_7_0_OEM_BRANCH

Comment 89

[laurel]

OK using sep12 commercial trunk build: win98,linux rh6.2
OK using sep10 commercial trunk build: mac OS 10.1

Changes being made to the filter UI as of sep16 and later will not build in
provision to cancel such changes on all platforms -- there will be no cancel
button, just a close button.  So, if Esc doesn't cancel the dialog on a given
platform there will be no cancel provision on the filter list dialog.