Closed Bug 532061 Opened 10 years ago Closed 10 years ago

Prohibit modification of property of root-level smart folder (same as property of virtual folder), unless customization of it is implemented

Categories

(Thunderbird :: Folder and Message Lists, enhancement)

enhancement
Not set

Tracking

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed)

RESOLVED FIXED
Tracking Status
blocking-thunderbird3.1 --- beta2+
thunderbird3.1 --- beta2-fixed

People

(Reporter: World, Assigned: Bienvenu)

References

Details

(Whiteboard: [needs updated patch])

Attachments

(2 files, 4 obsolete files)

At least prohibit modification of property of root-level smart folder(currently same as property of virtual folder), unless customization of it will be supported/implemented before release of Tb 3.
Or, confusion by many Tb3 users will be produced, then bugs like bug 520392, bug 531878, etc. will be opened frequently after release of Tb3.
If possible, different property for root-level smart folder from one for virtual folder is preferable.
Component: General → Folder and Message Lists
QA Contact: general → folders-message-lists
Flags: blocking-thunderbird3?
To me, this feels like the sort of thing that could wait for a point-release.
After discussion and a bit more thought, I don't think we'd actually block a point-release on this either, though it would be nice to get.  We might or might not block 3.1 on it, so setting that flag.
Flags: blocking-thunderbird3? → blocking-thunderbird3.1?
Flags: blocking-thunderbird3-
yeah, I wouldn't block a point release on this.
Summary: Prohibit modification of property of root-level smart folder(same as property of virtual folder), unless customization of it is implemented before release of Tb 3 → Prohibit modification of property of root-level smart folder (same as property of virtual folder), unless customization of it is implemented
I've met a newbie, Bug 533918.
See Also: → 507335
I understand the desire to eliminate confusion over a feature which is not completely implemented (perhaps was never intended to be released), but I think that the ability to edit the list of folders aggregated into a smart folder is quite valuable.  See  Bug #507335 (esp cmt 7) for rationale.  

If the decision is made to turn off the editing for now, please consider putting back a feature request to enable this in a future release.
FYI.
As of today, next bugs are DUP'ed to Bug 507335. 
  Bug 518530, Bug 520392, Bug 531878, Bug 533918, Bug 533947
I hope number of DUP of Bug 507335 won't exceed 100 after release of Tb 3.0(.0).
A newbie: bug 534784
I can hide this menu item, sure.
Assignee: nobody → bienvenu
Attached patch proposed fixSplinter Review
this hides the properties menu for smart folders - doing a mozmill test is going to take some time, however, since we don't have any tests for the folder pane context menus. So this may not make 3.01

For 3.1, I'd like to make editing smart mailboxes just work, but that requires more testing.
What about Edit -> Folder Properties?
ah, right, I'll have to do that as well.
blocking-thunderbird3.1: --- → beta2+
Flags: blocking-thunderbird3.1?
We should get this in .3 - its still confusing users.
blocking-thunderbird3.0: --- → .3+
bug 548864 has joined.
blocking-thunderbird3.0: .3+ → .4+
This takes out the code that recalculates the scope of smart mailboxes on startup, and fixes some of the code that tries to keep smart folders up to date when folders are added, in particular when sub-folders are created under the Archives and Sent folders. However, we still need to deal with things like folder flags changing (a folder losing or gaining the sent flag, for example).
Blocks: 507335
Duplicate of this bug: 549973
Duplicate of this bug: 507335
Whiteboard: [patch in progress]
Attached patch update on flag changes (obsolete) — Splinter Review
this sends notifications on flag changes, and makes the account manager listen for them, and update any relevant smart folders. That all seems to be working, but I did see a slight corruption of a smart folder uri list, so I need to look into that. This patch does fix the crash caused by that corruption, however, and since that's an existing crash in crash-stats, I suspect it's an existing bug.

I also need to write some unit tests for flag changing and smart folder scope updating.
Attachment #429637 - Attachment is obsolete: true
Keywords: relnote
There was some discussion/debate about what we'd do with this for 3.0.x. Given that I'm not sure we fully decided, pushing this out to think about next time round.
blocking-thunderbird3.0: .4+ → needed
Attached patch proposed fix (obsolete) — Splinter Review
This adds the notification needed for the listener to know when folder flags have changed, and makes the account manager update smart folder search scopes when folder flags change in a relevant way. It also makes the AM update search scopes when folders with smart flags are added or deleted. Finally, this adds some mozmill tests for detecting these cases...
Attachment #432708 - Attachment is obsolete: true
Attachment #435412 - Flags: superreview?(bugzilla)
Attachment #435412 - Flags: review?(bwinton)
Whiteboard: [patch in progress] → [has patch for review]
pinging for review - I'd really like this to get some testing and not land it at the last minute...
(In reply to comment #20)
> pinging for review - I'd really like this to get some testing and not land it
> at the last minute...

Happy to help, just need a pointer on how to apply the attached patch...
Comment on attachment 435412 [details] [diff] [review]
proposed fix

So you know that this is mainly going to be nits, right?  ;)

>+++ b/mail/test/mozmill/folder-tree-modes/test-smart-folders.js
>@@ -171,13 +172,89 @@ function test_select_folder_expands_coll
>   mc.folderTreeView.selectFolder(inboxSubfolder);
> 
>   assert_folder_collapsed(smartInboxFolder);
>   assert_folder_expanded(rootFolder);
>   assert_folder_selected_and_displayed(inboxSubfolder);
> }
> 
> /**
>+ * Test that smart folders are updated when the folders they should be
>+ * searching over are added/removed or have the relevant flag set/cleared.
>+ */
>+function test_folder_flag_changes() {
>+  expand_folder(smartInboxFolder);
>+  // Now attempt to select the folder

We should probably have a "." at the end of this comment.

>+  mc.folderTreeView.selectFolder(inboxSubfolder);
>+  // need to archive two messages  in two different accounts in order to

I think the first letter should be capitalized, and there is an extra space between "messages  in".

>+  let archiveScope = "|" + smartArchiveFolder.msgDatabase.dBFolderInfo
>+                    .getCharProperty("searchFolderUri") + "|";

I think this indent should contain one more space.

>+  // We should have both this account, and a folder corresponding
>+  // to this year in the scope.
>+  rootFolder = inboxFolder.server.rootFolder;
>+  let archiveFolder = rootFolder.getChildNamed("Archives");

So, we don't test localized builds, do we?  If we did, would this fail?

>+  assert_folder_and_children_in_scope(archiveFolder, archiveScope);

And we're not checking that we have a folder corresponding to the year, as far as I can tell.  I don't think we need to, but I thought it was worth mentioning.

>+  // Remove the archive flag, and make sure the archive folder and
>+  // its children are no longer in the search scope.
>+  archiveFolder.clearFlag(nsMsgFolderFlags.Archive);
>+  archiveScope = "|" + smartArchiveFolder.msgDatabase.dBFolderInfo
>+                    .getCharProperty("searchFolderUri") + "|";

And this indent should contain three fewer spaces.

Also, won't this give us the same archiveScope as the previous setting above?

>+function assert_folder_and_children_in_scope(folder, searchScope)
>+{
>+  let folderURI = "|" + folder.URI + "|";
>+  assert_uri_found(folderURI, searchScope);
>+  let allDescendents = Cc["@mozilla.org/supports-array;1"]
>+                     .createInstance(Ci.nsISupportsArray);

I think we need four more spaces in this indent.

>+function assert_uri_not_found(folderURI, scopeList)
>+{
>+  if (scopeList.indexOf(folderURI) != -1)
>+    throw new Error("scope " + scopeList + "contains (but shouldn't) " + folderURI);
>+}

I think this would read better as:
"scope " + scopeList + " contains " + folderURI + " but shouldn't."

>+++ b/mailnews/base/src/nsMsgAccountManager.cpp

Standard8, while I'm reviewing this code, I'm not an expert in it, and I don't think I'm a peer in this area, at least not yet.  ;)  So if you could concentrate on it a little more, that would probably be a good idea.

>@@ -3504,19 +3534,114 @@ NS_IMETHODIMP nsMsgAccountManager::OnIte
>-NS_IMETHODIMP nsMsgAccountManager::OnItemIntPropertyChanged(nsIMsgFolder *item, nsIAtom *property, PRInt32 oldValue, PRInt32 newValue)
>+NS_IMETHODIMP
>+nsMsgAccountManager::OnItemIntPropertyChanged(nsIMsgFolder *aFolder,
>+                                              nsIAtom *aProperty,
>+                                              PRInt32 oldValue,
>+                                              PRInt32 newValue)
> {
>-  return NS_ERROR_NOT_IMPLEMENTED;

Nice!  :)

>+  if (aProperty == mFolderFlagAtom)
>+  {
>+    PRUint32 flagsChanged = oldValue ^ newValue;
>+    const PRUint32 smartFolderFlags = nsMsgFolderFlags::Inbox |
>+                                nsMsgFolderFlags::Templates |
>+                                nsMsgFolderFlags::Trash |
>+                                nsMsgFolderFlags::Drafts |
>+                                nsMsgFolderFlags::Archive |
>+                                nsMsgFolderFlags::SentMail |
>+                                nsMsgFolderFlags::Junk;

I think these should line up with the nsMsgFolderFlags on the first line…

>+    if (flagsChanged & smartFolderFlags)
>+    {
>+      if (flagsChanged & newValue)
>+      {
>+        // if the smart folder flag was set, calling OnItemAdded will
>+        // do the right thing.

That doesn't seem to be the right check.  Don't you mean:
if (newValue & smartFolderFlags)
?  Uh, let's make this concrete.
oldValue = nsMsgFolderFlags::Inbox;
newValue = nsMsgFolderFlags::NonSpecialFolder;
So, flagsChanged == Inbox|NonSpecialFolder, cause they're not the same.
flagsChanged & smartFolderFlags == Inbox.
but, flagsChanged & newValue == NonSpecialFolder, even though the smart folder flag wasn't set.

>+      RemoveFolderFromSmartFolder(aFolder, flagsChanged & smartFolderFlags);
>+      // sent|archive flag removed, remove sub-folders from smart folder.
>+      if (flagsChanged & (nsMsgFolderFlags::Archive | nsMsgFolderFlags::SentMail))

Here too, I think you could add the Archive flag, and trigger this block.

>+nsresult
>+nsMsgAccountManager::RemoveFolderFromSmartFolder(nsIMsgFolder *aFolder,
>+                                                 PRUint32 flagsChanged)
>+{
>+  nsCString removedFolderURI;
>+  aFolder->GetURI(removedFolderURI);
>+  // Flag was removed. Look for smart folder based on that flag,
>+  // and remove this folder from its scope.

I don't think we can guarantee that the flag was removed at this point.

>+          // remove last '|' we added
>+          searchURI.SetLength(searchURI.Length() - 1);
>+
>+        // remove leading '|' we added (or one after |folderURI, if first URI)

This comment should have a little more indentation.

Okay, uh, it was mainly nits, but I'm a little concerned about the logic checks I mentioned above.  So I'm going to r- it for now, but if you can explain to me why they're right, I'll switch it to an r+.

Thanks,
Blake.
Attachment #435412 - Flags: review?(bwinton) → review-
Comment on attachment 435412 [details] [diff] [review]
proposed fix

The general idea of the account manager changes looks good, but I'll take a more detailed look when we get an updated patch.
Attachment #435412 - Flags: superreview?(bugzilla)
Whiteboard: [has patch for review] → [needs updated patch]
Attached patch fix addressing comments (obsolete) — Splinter Review
This addresses Blake's comments. Re the flag questions/comments, in reverse order:

> I don't think we can guarantee that the flag was removed at this point.

We only get into RemoveFolderFromSmartFolder if the flag has been removed, because we return out of OnFlagChanged if the flag was added before the call to RemoveFolderFromSmartFolder. But I've added an assertion just to be sure.

> That doesn't seem to be the right check.  Don't you mean:...

The code as it was worked because pretty much because only one smart folder flag ever changed at a time. But you're right that it was flawed, so I've changed it to make sure it's only dealing with the smart folder flag changes...
Attachment #435412 - Attachment is obsolete: true
Attachment #438154 - Flags: superreview?(bugzilla)
Attachment #438154 - Flags: review?(bwinton)
Whiteboard: [needs updated patch] → [needs review bwinton, sr standard8]
Comment on attachment 438154 [details] [diff] [review]
fix addressing comments

The changes look good to me.  As before, I'm not an expert in mailnews/base/src/nsMsgAccountManager.cpp, so if Standard8 gives it a little more attention while he's super-reviewing the patch, that would be cool.

Later,
Blake.
Attachment #438154 - Flags: review?(bwinton) → review+
Whiteboard: [needs review bwinton, sr standard8] → [needs sr standard8]
Comment on attachment 438154 [details] [diff] [review]
fix addressing comments

The mozmill test failed for me "Application unexpectedly closed" - did you really mean to go offline (you filed a bug on that), or did you include the wrong mozmill test file?

>+  // For Sent/Archives/Trash, we treat sub-folders of those folders as
>+  // "special", and want to add them the smart folders search scope.
>+  // So we check if this is a sub-folder of one of those special folders
>+  // and set the corresponding folderFlag if so.
>+  if (!addToSmartFolders)
>+  {
>+    PRBool isSpecial = PR_FALSE;
>+    folder->IsSpecialFolder(nsMsgFolderFlags::SentMail, PR_TRUE, &isSpecial);
>+    if (isSpecial)
>+    {
>+      addToSmartFolders = PR_TRUE;
>+      folderFlags |= nsMsgFolderFlags::SentMail;
>+    }
>+    folder->IsSpecialFolder(nsMsgFolderFlags::Archive, PR_TRUE, &isSpecial);
>+    if (isSpecial)
>+    {
>+      addToSmartFolders = PR_TRUE;
>+      folderFlags |= nsMsgFolderFlags::Archive;
>+    }

What about checking for trash? (as per the comment)
Attachment #438154 - Flags: superreview?(bugzilla) → superreview-
Whiteboard: [needs sr standard8] → [needs updated patch]
I included one too many mozmill tests. This patch doesn't have that extra test, and addresses your comment about the Trash folder.
Attachment #438154 - Attachment is obsolete: true
Attachment #439241 - Flags: superreview?(bugzilla)
Attachment #439241 - Flags: superreview?(bugzilla) → superreview+
fix checked in.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Cancelling flag for 3.0.x as I'm pretty sure we agreed we wouldn't disable this in 3.0.x - as although it isn't supported there, we don't want to teach users it isn't available when 3.1 fixes it.
blocking-thunderbird3.0: needed → ---
So, has the actual bug with the feature been fixed or have we swept it under the carpet by disabling the menu item altogether? If it's the latter, is there a ticket for the actual feature which I can watch/vote?
In 3.1, you can customize smart folders.
A newbie: bug 574910
I just installed 3.1, it works, thanks!
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.