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 ...

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.

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.

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)

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.

(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!

Agh, I keep hitting "take" on the QA Contact field lately.

No tests yet, but this seems to work...

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!

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.)

Ok, this should work and has some tests. Any ideas who would be good to do a full review of this?

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.)

Comment on attachment 8337334 [details] [diff] [review] Now with tests Switching reviewers since I flagged mconley for too many reviews already. :)

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

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

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

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

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

Re-landed: https://tbpl.mozilla.org/?tree=Thunderbird-Trunk&rev=12e6710c1ed6 Hopefully it'll stick this time!

Thanks!