Closed Bug 528044 Opened 15 years ago Closed 10 years ago

Global search: "Show/Open as list" results view does not remember columns (should persist column visiblity, e.g. for "location") [faceted search; search all messages]

Categories

(Thunderbird :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 29.0

People

(Reporter: tessarakt, Assigned: squib)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [gs][gssolved])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.9.1.5) Gecko/20091102 Firefox/3.1b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.9.1.6pre) Gecko/20091110 Lightning/1.0pre Shredder/3.0pre

If I enable a column, say "folder" (or what is "Ablageort" in en?), it should still be there on the next search.

Reproducible: Always

Steps to Reproduce:
1. Search for some terms in "all messages"
2. In the faceted search tab, "View as list"
3. Enable a column.
4. Close faceted search and list tab.
5. Search for some other terms.
6. "View as list"
Actual Results:  
Added column is gone

Expected Results:  
Column should still be shown.

There are some colums I specifically need in search results - esp. the containing folder, since messages are from different folders ...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Please flag this bug "status-Thunderbird: wanted+"

Small bug, big effect:

This bug makes it very hard to determine the location of found messages. Without location and account information, I cannot safely act on the message. It's completely annoying having to toggle the columns back on every time I search, and it's clearly a bug, because we usually do remember columns in the main window.

I'll add some some similar bugs to "See also" field, giving further evidence that users expect their columns and sortings to be remembered.
Severity: enhancement → normal
See Also: → 374551, 297251, 260289
Summary: "Show as list" should remember columns → "Show as list" faceted search results should remember columns
Version: unspecified → 3.0
Summary: "Show as list" faceted search results should remember columns → "Show as list" faceted search results does not remember columns (should persist column visiblity, e.g. for "location")
1 vote for this bug.  Its the first thing I have to do nearly every time I pull up search results.  Really useful column.  Really should be default for the search results but at least please make it stick.
Thanks!
The column I keep adding is "Location" ... curiously, my saved searches *do* show the location column by default. I didn't even tell them to do that. It's only in new searches where I have to enable the location column.
Lots of requests for this in Get Satisfaction as well.
Yup.  As they say in the restaurant business, "It's all about location".

Seriously, maybe someone can just add the Location column as default until this can more generally get resolved.  My guess is just about every user wants to know where the files are since the search results are across all folders.
Version: 3.0 → 3.1
Depends on: 570787
Per popular demand in above comments and on getsatisfaction (according to comment 5):

Bug 570787  - Global Search: "Open as list" Results should show location column by default [containing folder, faceted search, Search all messages]
Please vote for that bug if you like it (using vote button).
This will have a similar potential for user annoyance as related Bug 505035
(Cannot change the column list of all folders at once), as soon as more people
start using faceted search and discover the goodness of plain vanilla "open as
list" mode, compared to the glaring non-versatility of faceted search results
(we have some bugs for that, and getsatisfaction feedback, too).

Bug 570787 (show location column by default) might be a temporary easier workaround for the most frequent use case of this bug, which however would still suffer from:
Bug 522768 - Search Results: Missing full path in "Location" column (Faceted
Search: Open as List, Saved Searches, Search Messages; Main 3-pane Window)
See Also: → 522768
Summary: "Show as list" faceted search results does not remember columns (should persist column visiblity, e.g. for "location") → "Show/Open as list" global search results does not remember columns (should persist column visiblity in list view, e.g. for "location"; faceted search; search all messages)
Summary: "Show/Open as list" global search results does not remember columns (should persist column visiblity in list view, e.g. for "location"; faceted search; search all messages) → "Show/Open as list" global search results does not remember columns (should persist column visiblity in list view, e.g. for "location" [faceted search; search all messages]
Flags: in-testsuite?
Flags: in-litmus?
See Also: → 505035
Whiteboard: [gs]
Summary: "Show/Open as list" global search results does not remember columns (should persist column visiblity in list view, e.g. for "location" [faceted search; search all messages] → Global search: "Show/Open as list" results view does not remember columns (should persist column visiblity, e.g. for "location") [faceted search; search all messages]
moreia, please cite the getsatisfaction topic URL(s). TIA

probably half my usage of search results needs location column, so this borders on major for me.  Thomas, I don't see why 570787 isn't fundamentally a duplicate.
(In reply to comment #9)
> moreia, please cite the getsatisfaction topic URL(s). TIA
Thx Wayne, that's what I forgot to say.

> probably half my usage of search results needs location column, so this
> borders on major for me. 

Wayne! *shocked that he actually wants to /raise/ severity of a bug* ;)
Go ahead, be brave! ;) Seriously, you are right.

> Thomas, I don't see why 570787 isn't fundamentally a
> duplicate.

Well, fundamentally, the universe is one... ;)
...but essentially, we can have bug 570787 without having this one, and vice versa. And if we have this one (remember columns), we might still want bug 570787 (with some variant of bug 522768) to make the default set of columns more useful for the assumed majority of users who, like you and me, consider location column a must for any type of global search (where location should include full path, even if in a tooltip, not just folder name as we have now).
Just want to add that this issue also pertains to the "open in conversation" message view: custom column settings do not stick. Two birds one stone, perhaps.

In TB 3.1, in both the conversation view and the gloda search results view, the threadpane column picker misleadingly includes the menu item "Apply columns to..." even though selecting it does not do anything.
Flags: wanted-thunderbird?
Flags: wanted-thunderbird? → wanted-thunderbird+
(In reply to Moreia from comment #5)
> Lots of requests for this in Get Satisfaction as well.

Can somebody find those many reports mentioned by Moreia, and tag them with "Bug 528044"? That tag will automatically link them with the URL of this bug.
> Can somebody find those many reports mentioned by Moreia, and tag them with
> "Bug 528044"? That tag will automatically link them with the URL of this bug.

Done.
hurray!!!

finally that bug is fixed at least: #505035 (Cannot change the column list of all folders at once)

but this here is still a problem!
i cannot achieve the behaviour to see the location still!!

please fix this!!!!!!
If someone wants to fix this, I believe the best course of action is to modify folderDisplay.js to check 'isSynthetic' on the DBViewWrapper and then ask the synthetic view for the persisted column state string, if it has one.  It should likewise tell the synthetic view when the columns change so the synthetic view can decide whether to persist the changes.

In gloda's synthetic view, it would make sense to provide a name/type to the synthetic view, and then have gloda persist/depersist the data from that slot.  For example, 'global' could be used for this case, and 'conversation' for the "open in conversation" view.  I think it would be easiest to persist the information as a preference.

"mailnews.database.global.views.VIEWTYPE.columns" would be a reasonable location for this, I think.

In terms of the modifications to folderDisplay.js, the columns are usually depersisted in FolderDisplayWidget.onLoadingFolder which is triggered by DBViewWrapper._prepareToLoadView.  _prepareToLoadView is only invoked for folders with an actual message database, and onLoadingFolder strongly implies having a DBFolderInfo instance, so I don't think it makes sense to reuse those.

For depersistence, it might be easiest to modify FolderDisplayWidget.onDisplayingFolder in the clause that checks _savedColumnStates and then does the default column selection logic.  I would add the synthetic check there, do an additional check if the synthetic view (gloda's "dbview.js" in this case) implements the desired method (let's say "getPersistedSetting" which also takes a string argument which would be "columns") and then call that and use its value if returned.  I would have the columns passed as an object and have the persistence function itself call JSON.stringify rather than having FolderDisplayWidget pre-JSON things itself.  This would be useful for persistence cases like IndexedDB where structures clones are available and JSON is not required.

For persistence, I would just modify _persistColumnStates directly to do the synthetic check and have it check for "setPersistedSetting" and use that if it exists.
Taking this. I want to make Gloda awesome for TB31, and this seems like some good low-hanging fruit!
Status: NEW → ASSIGNED
QA Contact: squibblyflabbetydoo
Agh, I keep hitting "take" on the QA Contact field lately.
Assignee: nobody → squibblyflabbetydoo
QA Contact: squibblyflabbetydoo
Attached patch WIP patch (obsolete) — Splinter Review
No tests yet, but this seems to work...
Attachment #823051 - Flags: feedback?(bugmail)
Comment on attachment 823051 [details] [diff] [review]
WIP patch

I no longer have this stuff fresh in my brain, but I think I did when I wrote that bugzilla comment, and this seems to line up with that, so I think it looks good.  The potential double path for getting to defaults is not ideal, but I think it makes sense and the extra line is justified versus the potential hassle to try and clean up all the paths.

I very much look forward to this!
Attachment #823051 - Flags: feedback?(bugmail) → feedback+
Does anyone know if we have tests for the Gloda "Show as list" view? If so, where are they? I feel like this probably wants tests, but I'm not sure where to put them (although I wouldn't mind if someone said we don't need tests here!).
(In reply to Jim Porter (:squib) from comment #23)
> Does anyone know if we have tests for the Gloda "Show as list" view? If so,
> where are they? I feel like this probably wants tests, but I'm not sure
> where to put them (although I wouldn't mind if someone said we don't need
> tests here!).

I don't believe there are tests.  Specifically:
- No mozmill tests for the faceted search UI
- No xpcshell tests for mailnews/db/gloda/modules/dbview.js
- No xpcshell tests for the synthetic view support in FolderDisplayWidget/dbViewWrapper to any great detail.

In practice, http://mxr.mozilla.org/comm-central/source/mail/base/modules/MsgHdrSyntheticView.js may be tested by http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-message-window.js or something like it that uses MsgHdrSyntheticView.

Note: You may want to also updated that file too!

The closest set of existing tests to what you are doing is http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-columns.js.  One possibility is just adding to that file or cribbing from it.  While runtest.py for mozmill disables gloda, disabled gloda can still run queries, there will just be no results.  You could directly open one of the tabs with any gloda query.  Column persistence logic should still be triggered.  (You just can't go through the faceted search UI.)
Attached patch Now with tests (obsolete) — Splinter Review
Ok, this should work and has some tests. Any ideas who would be good to do a full review of this?
Attachment #823051 - Attachment is obsolete: true
Attachment #8337334 - Flags: feedback?(bugmail)
The code would seem to fall under the 3-pane sub-module, so :mconley would seem to be your existing, available reviewer.  Other options include new-ish contributor Andrew Buehler from bug 920510 (also folder display related), or Magnus Melin who reviewed that.

(The gloda code in mailnews/ is really just for the benefit of mail/ so I don't think you need to bother :protz for that.)
Attachment #8337334 - Flags: feedback?(bugmail) → review?(mconley)
Comment on attachment 8337334 [details] [diff] [review]
Now with tests

Switching reviewers since I flagged mconley for too many reviews already. :)
Attachment #8337334 - Flags: review?(mconley) → review?(mkmelin+mozilla)
Comment on attachment 8337334 [details] [diff] [review]
Now with tests

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

Looks like it does the job! r=mkmelin

::: mail/test/mozmill/folder-display/test-columns.js
@@ +448,5 @@
>    assert_visible_columns(conExtra);
>  }
>  test_apply_to_folder_and_children.EXCLUDED_PLATFORMS = ["linux"];
> +
> +var GLODA_DEFAULTS = [

const

@@ +464,5 @@
> +function FakeCollection() {
> +  this.items = [];
> +}
> +
> +function test_column_defaults_gloda_collection() {

please add documentation of what it tests (same with the other methods)

::: mailnews/db/gloda/modules/dbview.js
@@ +153,5 @@
> +  getDefaultSetting: function(aSetting) {
> +    if (aSetting == "columns")
> +      return this.DEFAULT_COLUMN_STATES;
> +    else
> +      return undefined;

no else after return please
Attachment #8337334 - Flags: review?(mkmelin+mozilla) → review+
Ok, I've addressed all the review comments, and I'll land this just as soon as the tree reopens.
Landed: https://hg.mozilla.org/comm-central/rev/dcc265385f91
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Flags: in-testsuite? → in-testsuite+
This may have caused mozmill failures on -trunk:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/folder-display/test-opening-messages.js | test-opening-messages.js::test_open_message_in_existing_window
TypeError: this.displayedFolder is null

It seems to be the single patch between a green run and today's failures (except the build fixes).
Backed out from trunk as this landed on a busted tree and possibly caused the failures (it could be in m-c, but looks unlikely based on the failures):

https://hg.mozilla.org/comm-central/rev/cb2e8c1d0782
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Fix the testsSplinter Review
Note to self: run full try builds when changing anything remotely related to folderDisplay.js or dbViewWrapper.js.

And here is said try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d1e49adbe4f5
Attachment #8337334 - Attachment is obsolete: true
Attachment #8365192 - Flags: review?(mkmelin+mozilla)
Attached patch The interdiffSplinter Review
The changes in dbViewWrapper.js aren't strictly necessary (I think), but they'll prevent similar errors in the future, and it's consistent with how the other getters work.
Comment on attachment 8365192 [details] [diff] [review]
Fix the tests

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

LGTM! r=mkmelin
Attachment #8365192 - Flags: review?(mkmelin+mozilla) → review+
Re-landed: https://tbpl.mozilla.org/?tree=Thunderbird-Trunk&rev=12e6710c1ed6

Hopefully it'll stick this time!
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Thanks!
Status: RESOLVED → VERIFIED
Whiteboard: [gs] → [gs][gssolved]
Blocks: 1028632
You need to log in before you can comment on or make changes to this bug.