Closed Bug 261199 Opened 18 years ago Closed 18 years ago

UI for defining and editing virtual folders

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird0.9

People

(Reporter: Bienvenu, Assigned: mscott)

References

Details

Attachments

(10 files, 3 obsolete files)

11.17 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
48.03 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
86.50 KB, image/png
Details
9.67 KB, patch
Details | Diff | Splinter Review
1.91 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.73 KB, patch
Details | Diff | Splinter Review
9.31 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
17.44 KB, image/png
Details
2.62 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.49 KB, patch
mscott
: review+
mscott
: superreview+
Details | Diff | Splinter Review
We need a UI for defining (File | New Virtual/Search Folder) and editing (folder
properties) virtual folders. I believe the same dialog can be used for both.

The dialog needs to allow the users to define/edit the search terms, and the
search scope (the folders to be searched over). I think to be really useful, we
should allow the users to pick an arbitrary set of folders to search over,
instead of a folder and optionally its subfolders. I think something like the
subscribe UI where the user can check/uncheck each folder would be good. 

It would be nice if this dialog also allowed the user to specify that the search
be done offline for imap and news folders, since searching a lot of imap folders
can be prohibitive on servers that don't index, like UW in its default
configuration. I have not implemented the backend for this but it should be trivial.

Right now, I don't know if we do pan-server searches in the backend, but I think
we might - it could just be a matter of adding new scopes, and the backend
should just chain them. But, I could be wrong. It would be great if we could
have pan-server virtual folders. Maybe later...

The problem with the subscribe UI approach is that we couldn't use RDF like the
current subscribe UI's, since the "search over" attribute isn't folder specific,
but depends on what the virtual folder itself is.

CreateVirtualFolder in commandglue.js shows you how to create a virtual folder
(basically the search terms and folder uris are stored in the dbfolderinfo of
the db... and accountManager.saveVirtualFolders() saves all that info in
virtualfolders.dat - if we could add a "seach offline boolean, we'd add that
there - I can do that, actually...)
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird0.9
sairuh was asking about this bug the other day
OS: Windows 2000 → All
Hardware: PC → All
Attached patch work in progress snapshot (obsolete) — Splinter Review
dialog for creating a new virtual folder. Not finished yet.

Still to do:

1) add a sub dialog for choosing the folders to search
2) add support for editing an existing virtual folder
3) add a check box for searching online or offline attributes
4) polish
Attachment #161118 - Attachment is obsolete: true
Attached patch update (obsolete) — Splinter Review
Progress update. Getting real close now. Everything is pretty functional. Just
down to the polish stage.

I haven't added some of the more advance feature requests david described like
select all sub folders and search online or offline yet.
Attachment #161124 - Attachment is obsolete: true
> select all sub folders and search online or offline yet.

With all this talk about selecting subfolders, I worry that recursive searches
in a folder where new subfolders are added wouldn't apply to those new
subfolders. Say it ain't so :)
I'm sorry to say that it is so - If we had some way of expressing a uri for all
sub-folders (e.g., mailbox://foo@bar/Inbox/*) then maybe we could do this. We
might make a special case for the account wide search, so if the search uri is
for an account, then we search all folders on the account...
breaking the patch down into pieces for an easier landing.
Comment on attachment 162168 [details] [diff] [review]
just the back end RDF data source changes

bienvenu super reviewed this portion this morning for me.
Attachment #162168 - Flags: superreview+
the RDF data source changes have landed on the branch and the trunk.
Attachment #162043 - Attachment is obsolete: true
Comment on attachment 162268 [details] [diff] [review]
patch for thunderbird ready for review

This makes the new UI work for Thunderbird. I haven't ported it to seamonkey
yet.

Not everything is finished yet, most noticeably the check box for toggling
between offline and online searching and a check box in the folder list dialog
for including subfolders (but you can manually choose as many folders as you
want).

I also haven't converted advanced search and the Views menu to use the new
dialog. Quick search has been converted to bring up a pre-filled version of the
new dialog.
Attachment #162268 - Flags: superreview?(bienvenu)
QA: Here is a summary of the new behavior

1) File / New / Saved Search Folder --> brings up new virtual folder UI

2) right clicking on an existing virtual folder, choose properties --> should
see virtual folder UI showing current VF properties. Title of the dialog should
reflect the name of the folder

3) Quick Search / Save As Search Folder --> should bring up new virtual foldere
UI with fields pre-filled for name, folder to create under, folder to search and
search terms.

4) If you attempt to create a new virtual folder and you don't choose any
folders to search, you should get an error dialog. 

5) Verbage has changed from Virtual Folders to Saved Search Folders in the UI

6) canceling the edit virtual properties dialog should not change any properties

7) canceling creationg of a new virtual folder should not do anything

8) After modifying the properties of an existing virtual folder, when you hit
ok, if that folder is the currenly loaded folder we should reload it, reflecting
the changes you made to the search criteria.

Known Issues still under development for this bug
--------------------------------------------------
1) Views / Save as Search Folder and Advanced Search / Save as Virtual Folder
need to use the new dialog and need language change for advanced search.

2) If you try to create a new virtual folder with a name that is already a valid
folder then we should bring up an error dialog.

3) The folders in the new folder picker dialog are not sorted correctly with the
special folders at the top like they are in the folder pane. I don't know why
yet. They are sorted alphabetically.

4) Need a checkbox in the folder picker dialog that says search sub folders.
Clicking on a folder would automatically select its subfolders for the virtual
folder search as well.

5) a checkbox in the main virtual folder dialog for doing an offline search
instead of searching online.

Those are most of the behavioral issues I can think of right now to pay
attention to when testing the new UI.
Comment on attachment 162268 [details] [diff] [review]
patch for thunderbird ready for review

thx so much for doing this! sr=bienvenu, with some nits:

     if (isServer || isInbox)
-      SetMenuItemLabel("menu_NewFolder",
-	 gMessengerBundle.getString("newFolder"));
+      SetMenuItemLabel("menu_NewFolder",
gMessengerBundle.getString("newFolder"));
     else
-      SetMenuItemLabel("menu_NewFolder",
-	 gMessengerBundle.getString("newSubfolder"));
+      SetMenuItemLabel("menu_NewFolder",
gMessengerBundle.getString("newSubfolder"));

this could just be:

SetMenuItemLabel("menu_newFolder", gMessengerBundle.getString((isServer ||
isInbox) ? "newFolder" : "newSubfolder");


Should this commented out code just be removed? :
+
+//  if (window.arguments[0].preselectedURI) {
+//    var uri = window.arguments[0].preselectedURI;
+//    var folder = GetMsgFolderFromUri(uri, true);
+//  }


+function onCancel()
+{
+  // generateFoldersToSearchList will undo all of our folder property changes
for us
+  generateFoldersToSearchList(); // ignore return string 
+  return;

you don't need the return;, right?

this is a little confusing. It seems like it could be written as one if,
assuming isContainerOpen doesn't have some side effect we're counting on: -
maybe 

if (obj.value != "twisty" && col.value == "selectedColumn")
  ReverseStateFromNode(row.value)


+    if (obj.value == "twisty") {
+	 if (gFolderPickerTree.view.isContainerOpen(row.value)) {
+	 }
+    }
+    else {
+      // if the user single clicks on the search folder check box, we handle
it here
+      if (col.value == "selectedColumn")
+	 ReverseStateFromNode(row.value);

here's a trick I learned from Brendan, I think:

+    if (arguments.searchTermSession)
+      gSearchTermSession = arguments.searchTermSession;
+    else
+      gSearchTermSession =
Components.classes[searchSessionContractID].createInstance(Components.interface
s.nsIMsgSearchSession);

can just be:

gSearchTermSession = arguments.searchTermSession ||
Components.classes[searchSessionContractID].createInstance(Components.interface
s.nsIMsgSearchSession);

in +function onOK()

I don't think you want +    msgDatabase.summaryValid = true;

since you're editing an existing folder...
Attachment #162268 - Flags: superreview?(bienvenu) → superreview+
This patch converts the advanced search / Save as Virtual Folder button
and the
Views / Save As Search Folder menu item

to use the new virtual folder dialog UI

Unfortunately this can't be checked into the trunk until I back port the new UI
to seamonkey since it's changing shared code.

But this patch and this behavior has now been checked into the aviary 1.0
branch for Thunderbird.
If you try to create a new virtual folder with a folder name that already
exists at the same level in your folder hierarchy as the new virtual folder
then alert the user and don't create the virtual folder.
Comment on attachment 162441 [details] [diff] [review]
check to see if a folder exists with the provided name and alert the user if so

David is nsIMsgFolder.containsChildName the right way to check if a folder
exists with the name the user provided? Do I need to do anything like escape
the user folder name or lower case it before calling this? It doesn't look like
it.
Attachment #162441 - Flags: superreview?(bienvenu)
Comment on attachment 162441 [details] [diff] [review]
check to see if a folder exists with the provided name and alert the user if so

containsChildNamed would mostly work, but it does a case-insensitive
comparison, which isn't quite right, especially for IMAP (though it errs on the
side of caution, so it's OK). I don't think you need to escape the name since
it's unicode, and we're comparing it with the unicode display name.

GetChildWithURI allows you to say whether you want a case-sensitive comparison
or not. If you pass in False for deep, it should do what you want (assuming you
can create the child uri at the point you want to call this...)
Attachment #162441 - Flags: superreview?(bienvenu) → superreview+
I just glanced at the screen shots for the UI elements.  There might be some
confusion as to what is a real folder and what is a virtual (saved search)
folder.  A magnifying glass or some other slightly different icon for the
virtual folder would help avoid any unnecessary confusion.

add CSS rules for the dialog that lets you pick which folders to search for OS
X.
re comment #19: that's covered by bug 263989.
Who says I can't write code while watching the Red Sox....
Comment on attachment 162533 [details] [diff] [review]
modify folder list picker dialog to only let you choose folders from the same account

David, this patch adds an account combo box at the top of the folder picker
dialog. You have to choose an account and then choose the folders to search on
that account. This prevents you from selecting folders across many accounts.
Attachment #162533 - Flags: superreview?(bienvenu)
Comment on attachment 162533 [details] [diff] [review]
modify folder list picker dialog to only let you choose folders from the same account

who says I can't review code while watching the red sox with my eyes closed?
:-)
Attachment #162533 - Flags: superreview?(bienvenu) → superreview+
Depends on: 265014
Please note that 265014 was just fixed which allows you to search online or
offline for online servers. Faster results if you search offline but you may get
stale data for imap and news servers.
FYI, there are two remaining issues keeping this bug open for branch and trunk:

1) The folders in the new folder picker dialog are not sorted correctly with the
special folders at the top like they are in the folder pane. I don't know why
yet. They are sorted alphabetically.

2) Need a checkbox in the folder picker dialog that says search sub folders.
Clicking on a folder would automatically select its subfolders for the virtual
folder search as well.

I may punt on issue #2 for now but would still like to fix issue #1.
I may be overlooking something obvious, but should this be working on SeaMonkey
trunk right now, or is it checked in yet dormant?

I see this was checked in on 2004-10-18 20:08, but my 2004102013 build shows no
evidence of File | New | Saved Search Folder under Messenger.
I have two additional nagging things with the folder picker dialog (using Build
ID=2004102022):

1. The account combobox *appears* empty while the selection of an account still
works surprisingly (also reported here:
http://forums.mozillazine.org/viewtopic.php?p=886915#886915 , but I don't see
the last account name here).

2. A "Select all folders" button that places the checkmark to all folders. My
ISP's IMAP server allows no hierarchy, so the "search subfolders" wouldn't help.
Thomas: Regarding Item #1, this is a known issue that Scott is working on. Stay
tuned.

(In reply to comment #29)
> I have two additional nagging things with the folder picker dialog (using Build
> ID=2004102022):
> 
> 1. The account combobox *appears* empty while the selection of an account still
> works surprisingly (also reported here:
> http://forums.mozillazine.org/viewtopic.php?p=886915#886915 , but I don't see
> the last account name here).
> 
> 2. A "Select all folders" button that places the checkmark to all folders. My
> ISP's IMAP server allows no hierarchy, so the "search subfolders" wouldn't help.

Attached patch RDF magic Splinter Review
RDF magic patch (don't claim to understand it) that fixed the last two big UI
nits:

1) Folders are now sorted properly in the folder picker dialog
2) The account picker in the folder picker dialog now shows the account names
in the drop down instead of using a height of 0 pixels for each row in the menu
list.
Attachment #162955 - Flags: superreview?(bienvenu)
Attachment #162955 - Flags: superreview?(bienvenu) → superreview+
Closing out this bug now that those last two issues have been addressed. If
there are any new problems with the new Thunderbird UI, we'll file new bugs now.
Thunderbird and saved search folders woo hooo!
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(In reply to comment #6)
> I'm sorry to say that it is so - If we had some way of expressing a uri for all
> sub-folders (e.g., mailbox://foo@bar/Inbox/*) then maybe we could do this. We
> might make a special case for the account wide search, so if the search uri is
> for an account, then we search all folders on the account...

Well you could adopt a glob idiom used in various other places: foo/* means all
immediate subfolders of foo, and foo/** means subfolders at any depth.

And later you might allow: foo/**/New or foo/**/!deleted etc.

Just a syntax idea FYI.

Version: unspecified → Trunk
[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

1 format + 1 reorder + 1 fix.

Could you (super-)review/check in this patch ? Thanks.
Attachment #177006 - Flags: superreview?(mscott)
Attachment #177006 - Flags: review?(mscott)
Attachment #177006 - Flags: superreview?(mscott)
Attachment #177006 - Flags: superreview+
Attachment #177006 - Flags: review?(mscott)
Attachment #177006 - Flags: review+
Whiteboard: [checkin-needed]
Comment on attachment 177006 [details] [diff] [review]
(Mv1) <widgetglue.js>
[Checked in: Comment 36]

'approval1.1a2=?':
Trivial JS code cleanup, no risk.
Attachment #177006 - Flags: approval-aviary1.1a2?
Attachment #177006 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Comment on attachment 177006 [details] [diff] [review]
(Mv1) <widgetglue.js>
[Checked in: Comment 36]


Check in: { 2005-06-14 18:21	timeless%mozdev.org	mozilla/ mail/ base/
content/ widgetglue.js	   1.14 }
Attachment #177006 - Attachment description: (Mv1) <widgetglue.js> → (Mv1) <widgetglue.js> [Checked in: Comment 36]
Attachment #177006 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
Comment on attachment 177006 [details] [diff] [review]
(Mv1) <widgetglue.js>
[Checked in: Comment 36]

patch is *not* "obsolete"
Attachment #177006 - Attachment is obsolete: false
No longer blocks: 298841
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.