Closed
Bug 1195480
Opened 9 years ago
Closed 9 years ago
Enable custom columns for secondary sort, persist and restore all column primary and secondary sort states
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(thunderbird42 unaffected, thunderbird43 fixed, thunderbird44 fixed)
RESOLVED
FIXED
Thunderbird 44.0
Tracking | Status | |
---|---|---|
thunderbird42 | --- | unaffected |
thunderbird43 | --- | fixed |
thunderbird44 | --- | fixed |
People
(Reporter: alta88, Assigned: alta88)
References
Details
Attachments
(5 files, 8 obsolete files)
8.92 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
11.12 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
12.21 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
10.47 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
1.17 KB,
patch
|
mkmelin
:
review+
rkent
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Enable multi custom column sorts, expose the secondary cust col, make sure sort order of primary is preserved when it becomes the secondary sort.
Attachment #8649868 -
Flags: review?(rkent)
Part 2, fix persistence for threading/sort info for the single virtual folder case; there is no change in behavior using the quick filter (currently any change while it's active is persisted for all folder types).
Attachment #8649993 -
Flags: review?(rkent)
Turning on the grouped by sort menuitem for custom columns, as well as expandAll/collapseAll for grouped, will be done in bug 1192838. There are some ui nits, like, it might be nice to list registered custom columns automatically in the View-Sort By menu. There are 2 situations left here: 1. multifolder saved searches do not re-sort when changing threaded/unthreaded, like single folders do. 2. non virtual folders build the view before custom columns are registered, ie the issue in bug 1192696, so are in limbo state despite the info being persisted and restored now.
tweaked.
Attachment #8649993 -
Attachment is obsolete: true
Attachment #8649993 -
Flags: review?(rkent)
Attachment #8651031 -
Flags: review?(rkent)
tweak, make some inline code reusable.
Attachment #8651031 -
Attachment is obsolete: true
Attachment #8651031 -
Flags: review?(rkent)
Attachment #8651216 -
Flags: review?(rkent)
Fix #2 in comment 3 above, #1 will be fixed in Bug 1192838. This should finish the pieces in this bug.
Attachment #8651848 -
Flags: review?(rkent)
Part 4, function rename per Bug 1192696.
Attachment #8652848 -
Flags: review?(rkent)
Comment 8•9 years ago
|
||
Comment on attachment 8649868 [details] [diff] [review] custColSecondarySortPersist.patch Review of attachment 8649868 [details] [diff] [review]: ----------------------------------------------------------------- I'm still looking at this. Do you understand why there are two sets of variables, m_sortOrder and m_sortColumns[0].mSortOrder? (Same question of course for many other similar variables). ::: mailnews/base/public/nsIMsgDBView.idl @@ +478,5 @@ > + * not byCustom. The secondary sort design is such that the desired secondary > + * is sorted first, followed by sort by desired primary. The secondary is > + * read only, as it is set internally according to this design. > + */ > + readonly attribute AString secondaryCustomColumn; You need to roll the UUID for this interface if you modify it like this. ::: mailnews/base/src/nsMsgDBView.cpp @@ +4161,5 @@ > + m_secondarySort = m_sortColumns[1].mSortType; > + m_secondarySortOrder = m_sortColumns[1].mSortOrder; > + m_secondaryCustomColumn = m_sortColumns[1].mCustomColumnName; > + } > + This method, DecodeColumnSort, has as its function to take a string, and use it to construct new elements in m_sortColumns. It is not handling the setting of other member variables such as m_curCustomColumn. Adding the secondary sort member variable sets here confuses the previously clear definition of the purpose of this method, which makes for more confusing code in the long run. I think that you should move this instead to occur right after the only call to DecodeColumnSort, which occurs in RestoreSortInfo(). In doing that, we see that another entry dealing with custom columns, the m_curCustomColumn, is being set from folderInfo at this point, rather than from m_sortColumns[0].mCustomColumnName. Do you know why secondary columns are handled differently here?
Comment 9•9 years ago
|
||
Answering my own question, here is what I think is the issue. In https://bugzilla.mozilla.org/show_bug.cgi?id=57898#c47 Bienvenu had secondary sort mostly working, and partly implemented is a multi-level sort. I think that the m_secondary... fields represent one design (only supporting two level search) while the m_sortColumns is the partially implemented multi-level search. This leaves us with an unnecessarily complex design that supports never-finished features. I don't expect that to be cleaned up in this bug, all that I expect is that we don't make it worse than it already is. Let me look at the code changes in that light.
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #8) > Comment on attachment 8649868 [details] [diff] [review] > custColSecondarySortPersist.patch > > Review of attachment 8649868 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm still looking at this. > > Do you understand why there are two sets of variables, m_sortOrder and > m_sortColumns[0].mSortOrder? (Same question of course for many other similar > variables). > I speculate that secondary sorts were bolted on long after there was just sortType and sortOrder stored in the db and never rationalized. So m_sortColumns was invented to keep track of 2 (but could be more) sorts and designed to push the first into the second index of m_sortColumns. It may be possible to only use the "sortColumns" property I but did not approach this with the idea of radically altering 15+ yr old design/wax buildup. Although EncodeColumnSort is not pleasant. > ::: mailnews/base/public/nsIMsgDBView.idl > @@ +478,5 @@ > > + * not byCustom. The secondary sort design is such that the desired secondary > > + * is sorted first, followed by sort by desired primary. The secondary is > > + * read only, as it is set internally according to this design. > > + */ > > + readonly attribute AString secondaryCustomColumn; > > You need to roll the UUID for this interface if you modify it like this. > Sure. > ::: mailnews/base/src/nsMsgDBView.cpp > @@ +4161,5 @@ > > + m_secondarySort = m_sortColumns[1].mSortType; > > + m_secondarySortOrder = m_sortColumns[1].mSortOrder; > > + m_secondaryCustomColumn = m_sortColumns[1].mCustomColumnName; > > + } > > + > > This method, DecodeColumnSort, has as its function to take a string, and use > it to construct new elements in m_sortColumns. It is not handling the > setting of other member variables such as m_curCustomColumn. Adding the > secondary sort member variable sets here confuses the previously clear > definition of the purpose of this method, which makes for more confusing > code in the long run. > > I think that you should move this instead to occur right after the only call > to DecodeColumnSort, which occurs in RestoreSortInfo(). Sure. > > In doing that, we see that another entry dealing with custom columns, the > m_curCustomColumn, is being set from folderInfo at this point, rather than > from m_sortColumns[0].mCustomColumnName. Do you know why secondary columns > are handled differently here? So custom columns were yet another bolt on, that came much after m_sortColumns, per cvs, and their info was just inserted into the MsgViewSortColumnInfo object to work with the secondary sort push to next index mechanism. Since DecodeColumnSort was never exposed, it was just decided to set a separate property for the custom column to be easily retrieved, I guess. I'll update once all comments are in/finalized.
Assignee | ||
Comment 11•9 years ago
|
||
Yes, I'd say the changes in these patches are really tweaks to make the existing design work better.
Comment 12•9 years ago
|
||
Comment on attachment 8649868 [details] [diff] [review] custColSecondarySortPersist.patch Review of attachment 8649868 [details] [diff] [review]: ----------------------------------------------------------------- I spent quite a bit of time looking at this, because I was (and am still) bothered by the unclear split in meaning between the member variables and the equivalent values in m_sortColumns. I toyed with some cleanup and clarification, but I gave up as not worth the risk. So this is fine with the nits fixed. ::: mailnews/base/src/nsMsgDBView.cpp @@ +4453,5 @@ > > +NS_IMETHODIMP nsMsgDBView::Sort(nsMsgViewSortTypeValue sortType, > + nsMsgViewSortOrderValue sortOrder) > +{ > + Nit: please do not add the extra blank line here. @@ +4461,5 @@ > + // Otherwise, to be on the safe side, resort. > + if (m_sortType == sortType && m_sortValid && > + (sortType != nsMsgViewSortType::byCustom || > + (sortType == nsMsgViewSortType::byCustom && m_sortColumns.Length() && > + m_sortColumns[0].mCustomColumnName.Equals(m_curCustomColumn))) && Here (and also below) you are interpreting m_sortColumns[0].mCustomColumnName as containing the colName for the previously done sort. Please add a comment here to help understand that. I suggest: // Note m_curCustomColumn is the desired (possibly new) custom column name, while m_sortColumns[0].mCustomColumnName is the name for the last completed sort, since these are persisted after each sort.
Attachment #8649868 -
Flags: review?(rkent) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8651848 [details] [diff] [review] checkCustColRegistered.patch Review of attachment 8651848 [details] [diff] [review]: ----------------------------------------------------------------- There's a serious issue with this that needs a solution. If you setup a sort using a custom column, and then that addon gets disabled, then there is never a registration of the custom column, and the sort never occurs. You are also unable to change the sort, as the persistence of the sort happens in the (never run) sort. Sort is broken forever on that folder, to fix you have to reenable the addon (but you don't actually know that's why it is failing). This needs fixing to proceed. Ideas? Perhaps you could set a timer that fires after a couple of seconds, and start the search anyway if it has not started yet. ::: mailnews/base/src/nsMsgDBView.cpp @@ +1931,5 @@ > } > > +// Check if any active sort columns are custom. If none are custom, return false > +// and go on as always. If any are custom, and all are not registered yet, > +// return true and don't sort. When the custom column observer is notified with Nit: return true (so that caller can postpone sort). ... (This method does not sort, and the statement as written implies that you do). @@ +1944,5 @@ > + > + bool custColNotRegistered = false; > + for (uint32_t i = 0; i < m_sortColumns.Length() && !custColNotRegistered; i++) > + { > + MsgViewSortColumnInfo &sortInfo = m_sortColumns[i]; You don't need this, just use in subsequent lines m_sortColumns[i].mSortType etc. Maybe the existing pattern is a relic of a previous array implementation.
Attachment #8651848 -
Flags: review?(rkent) → review-
Comment 14•9 years ago
|
||
Comment on attachment 8652848 [details] [diff] [review] renameGetCurColumnHandlerFromDBInfo.patch Review of attachment 8652848 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8652848 -
Flags: review?(rkent) → review+
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #13) > Comment on attachment 8651848 [details] [diff] [review] > checkCustColRegistered.patch > > Review of attachment 8651848 [details] [diff] [review]: > ----------------------------------------------------------------- > > There's a serious issue with this that needs a solution. > > If you setup a sort using a custom column, and then that addon gets > disabled, then there is never a registration of the custom column, and the > sort never occurs. You are also unable to change the sort, as the > persistence of the sort happens in the (never run) sort. Sort is broken > forever on that folder, to fix you have to reenable the addon (but you don't > actually know that's why it is failing). > > This needs fixing to proceed. Ideas? Perhaps you could set a timer that > fires after a couple of seconds, and start the search anyway if it has not > started yet. > Hmm, I thought this was handled but it looks like not. I think the solution should be deterministic both on the initial restore as now (rather than timer and assume registration after a bit) and also if there's never a registration. If the user removes a cust col, the next folder enter will be a byNone sort which is fine, so then they choose another sort and that overrides the persisted cust col by the new sort. I'll think about the mechanics of it..
Comment 16•9 years ago
|
||
(In reply to alta88 from comment #5) > Created attachment 8651216 [details] [diff] [review] > restoreSIngleVirtual.patch > > tweak, make some inline code reusable. I looked at this today, but have not finished. By first reaction is that there are no similar cases in folderDisplay.js where there are calls like: this.view.dbView.doCommand(Ci.nsMsgViewCommandType.collapseAll); so I am suspicious that perhaps this is not the correct place to do that. Is there any fundamental reason why this has to be fixed with a band aid like this, and not fixed so that the view is opened correctly in the first place?
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #16) > (In reply to alta88 from comment #5) > > Created attachment 8651216 [details] [diff] [review] > > restoreSIngleVirtual.patch > > > > tweak, make some inline code reusable. > > I looked at this today, but have not finished. > > By first reaction is that there are no similar cases in folderDisplay.js > where there are calls like: > > this.view.dbView.doCommand(Ci.nsMsgViewCommandType.collapseAll); > > so I am suspicious that perhaps this is not the correct place to do that. Is > there any fundamental reason why this has to be fixed with a band aid like > this, and not fixed so that the view is opened correctly in the first place? Well, the larger question is why have a heavy layer like folderDisplay and dbViewWrapper to couch things that can be done on/in gDBView directly. And while asuth can tell us for sure, it's likely in part to abstract the various subtle quirks of the different view classes. And avoid opening up the cpp, moving toward script. For example, restoreSelection() is recreated while nsMsgDBView::RestoreSelection is still there (and quirky). And the sort/threading code for creating the views' array is way more fragile and high risk than RestoreSelection. I did try to stick an nsMsgDBView::ExpandAll() in there but it blew up and wasn't pursued since the current alternative is so much easier. Maybe restoreThreadState() could be in dbViewWrapper but it was put in folderDisplay to be near its older sibling restoreSelection(); not that important imo. The nsMsgQuickSearchDBView class is the most quirky to the point I tried replacing it by opening a single saved search folder as xfvf (which works well) and it was ok on a small folder but permanently pegged all cores on a 70k message folder, and wasn't pursued further. I certainly get your point and would argue it in other situations, but in this case, don't think it's risk/reward/cost worth it.
Assignee | ||
Comment 18•9 years ago
|
||
This ensures restored non existent cust cols are cleaned up (in any combination of cust cols in a sort), and that the next user sort works, plus addresses the other comments.
Attachment #8649868 -
Attachment is obsolete: true
Attachment #8654536 -
Flags: review?(rkent)
Assignee | ||
Comment 19•9 years ago
|
||
put back wrongly obsoleted r+ patch.
Attachment #8651848 -
Attachment is obsolete: true
Attachment #8654537 -
Flags: review+
Assignee | ||
Comment 20•9 years ago
|
||
unbitrot this patch.
Attachment #8652848 -
Attachment is obsolete: true
Attachment #8654538 -
Flags: review+
Comment 21•9 years ago
|
||
Comment on attachment 8651216 [details] [diff] [review] restoreSIngleVirtual.patch Review of attachment 8651216 [details] [diff] [review]: ----------------------------------------------------------------- I spent way too much time looking at this, and ultimately I convinced myself that fixing the root cause (lack of support for expanded/collapsed view flags in quick search view) is more trouble than it is worth. I think that this view code is going to have to be completely rewritten in the next few years anyway due to upcoming changes in XUL, and this change seems to work, so let's do it. You do need the fix the Ci issue though, simple fix but serious problem. ::: mail/base/content/folderDisplay.js @@ +364,5 @@ > + return; > + > + if (this.view._threadExpandAll && > + !(this.view.dbView.viewFlags & nsMsgViewFlagsType.kExpandAll)) > + this.view.dbView.doCommand(Ci.nsMsgViewCommandType.expandAll); You cannot assume that Ci is defined in core code, and in fact it is not defined when I right click on a folder and ask to do a search. This missing Ci breaks doing that search. You can just define it locally in this function. ::: mailnews/base/src/nsMsgDBView.cpp @@ +4359,5 @@ > + folderInfo->GetProperty("sortColumns", sortColumnsString); > + DecodeColumnSort(sortColumnsString); > + > + // Restore curCustomColumn from db. > + folderInfo->GetProperty("customSortCol", m_curCustomColumn); From earlier reviews, I assume you are going to move the m_secondarySort... variable settings here as well.
Attachment #8651216 -
Flags: review?(rkent) → review+
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #21) > Comment on attachment 8651216 [details] [diff] [review] > restoreSIngleVirtual.patch > > Review of attachment 8651216 [details] [diff] [review]: > ----------------------------------------------------------------- > > I spent way too much time looking at this, and ultimately I convinced myself > that fixing the root cause (lack of support for expanded/collapsed view > flags in quick search view) is more trouble than it is worth. I think that > this view code is going to have to be completely rewritten in the next few > years anyway due to upcoming changes in XUL, and this change seems to work, > so let's do it. > Yes, maybe even more than I did, but I was convinced of both points going in... > You do need the fix the Ci issue though, simple fix but serious problem. > > ::: mail/base/content/folderDisplay.js > @@ +364,5 @@ > > + return; > > + > > + if (this.view._threadExpandAll && > > + !(this.view.dbView.viewFlags & nsMsgViewFlagsType.kExpandAll)) > > + this.view.dbView.doCommand(Ci.nsMsgViewCommandType.expandAll); > > You cannot assume that Ci is defined in core code, and in fact it is not > defined when I right click on a folder and ask to do a search. This missing > Ci breaks doing that search. > > You can just define it locally in this function. > Sure. > ::: mailnews/base/src/nsMsgDBView.cpp > @@ +4359,5 @@ > > + folderInfo->GetProperty("sortColumns", sortColumnsString); > > + DecodeColumnSort(sortColumnsString); > > + > > + // Restore curCustomColumn from db. > > + folderInfo->GetProperty("customSortCol", m_curCustomColumn); > > From earlier reviews, I assume you are going to move the m_secondarySort... > variable settings here as well. Yes, it's done in the final r? patch remaining.
Assignee | ||
Comment 23•9 years ago
|
||
udpated for comments.
Assignee: nobody → alta88
Attachment #8651216 -
Attachment is obsolete: true
Attachment #8656944 -
Flags: review+
Comment 24•9 years ago
|
||
Comment on attachment 8654536 [details] [diff] [review] checkCustColRegistered.patch Review of attachment 8654536 [details] [diff] [review]: ----------------------------------------------------------------- OK works fine now, r+ with nits fixed. ::: mailnews/base/src/nsMsgDBView.cpp @@ +1933,5 @@ > > +// Check if any active sort columns are custom. If none are custom, return false > +// and go on as always. If any are custom, and all are not registered yet, > +// return true (so that the caller can postpone sort). When the custom column > +// observer is notified with MsgCreateDBView and registers the handler; Nit: end with comma not semicolon. @@ +4420,5 @@ > +{ > + if (!m_sortColumns.Length()) > + return; > + > + nsString blank; You don't need this "blank" variable, see below. @@ +4427,5 @@ > + if (m_sortColumns[i].mSortType == nsMsgViewSortType::byCustom && > + m_sortColumns[i].mColHandler == nullptr) > + { > + m_sortColumns[i].mSortType = nsMsgViewSortType::byDate; > + m_sortColumns[i].mCustomColumnName = blank; You don't need the variable "blank", just use here: m_sortColumns[i].mCustomColumnName.Truncate() @@ +4430,5 @@ > + m_sortColumns[i].mSortType = nsMsgViewSortType::byDate; > + m_sortColumns[i].mCustomColumnName = blank; > + // There are only two... > + if (i == 0 && m_sortType != nsMsgViewSortType::byCustom) > + SetCurCustomColumn(blank); Instead of blank here, use: SetCurCustomColumn(EmptyString()); @@ +4432,5 @@ > + // There are only two... > + if (i == 0 && m_sortType != nsMsgViewSortType::byCustom) > + SetCurCustomColumn(blank); > + if (i == 1) > + m_secondaryCustomColumn = blank; same here, .Truncate()
Attachment #8654536 -
Flags: review?(rkent) → review+
Assignee | ||
Comment 25•9 years ago
|
||
thanks. (btw, many +s for the governance post)
Attachment #8654536 -
Attachment is obsolete: true
Attachment #8662444 -
Flags: review+
Keywords: checkin-needed
Comment 26•9 years ago
|
||
renameGetCurColumnHandlerFromDBInfo.patch fails to apply.
Keywords: checkin-needed
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8654538 -
Attachment is obsolete: true
Attachment #8662498 -
Flags: review+
Assignee | ||
Comment 28•9 years ago
|
||
hi aleth, thanks for all the checkins. this one applies locally, with current tip, and the other 3 patches. if not, please post the error file.
Keywords: checkin-needed
Comment 29•9 years ago
|
||
(In reply to alta88 from comment #28) > hi aleth, thanks for all the checkins. this one applies locally, with > current tip, and the other 3 patches. if not, please post the error file. For future reference, if there's a particular order in which the 4 patches need to be applied, it's useful to number them ;) When you import from this bug, you see 1: custColSecondarySortPersist.patch alta88: review+ 2: renameGetCurColumnHandlerFromDBInfo.patch alta88: review+ 3: restoreSIngleVirtual.patch alta88: review+ 4: checkCustColRegistered.patch alta88: review+ Which patches do you want to import, and in which order? [Default is all]
Assignee | ||
Comment 30•9 years ago
|
||
hmm, i guess bugzilla reorders them based on update and not initial submission order. if all doesn't work, it should be 1, 3, 4, 2 above which is the patch order in my queue. and the checkin comment is numbered for each.. thanks.
Comment 31•9 years ago
|
||
http://hg.mozilla.org/comm-central/rev/ae189fe20938 http://hg.mozilla.org/comm-central/rev/46985587445b http://hg.mozilla.org/comm-central/rev/26042be43bf1 http://hg.mozilla.org/comm-central/rev/05e9eb680cc6
Comment 32•9 years ago
|
||
This may have caused a whole bunch of mozmill test failures - please check.
Flags: needinfo?(alta88)
Assignee | ||
Comment 33•9 years ago
|
||
yes, standalone windows are always good for a trip up. this passes all mozmill other than the known permissions problem.
Attachment #8663472 -
Flags: review?(aleth)
Comment 34•9 years ago
|
||
Comment on attachment 8663472 [details] [diff] [review] fixtests.patch Review of attachment 8663472 [details] [diff] [review]: ----------------------------------------------------------------- I don't know this code, sorry.
Attachment #8663472 -
Flags: review?(aleth) → review?(rkent)
Comment 35•9 years ago
|
||
Does this maybe fix any of bug 1037857 or bug 57898?
Comment 36•9 years ago
|
||
Comment on attachment 8663472 [details] [diff] [review] fixtests.patch Maybe mkmelin would be better for this UI code.
Attachment #8663472 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 37•9 years ago
|
||
can the first person here with auth rs this half line test bustage fix and land before merge? otherwise it will need to be uplifted.
Comment 38•9 years ago
|
||
Comment on attachment 8663472 [details] [diff] [review] fixtests.patch Review of attachment 8663472 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable to me. r=mkmelin (missed the merge though :/)
Attachment #8663472 -
Flags: review?(rkent)
Attachment #8663472 -
Flags: review?(mkmelin+mozilla)
Attachment #8663472 -
Flags: review+
Keywords: checkin-needed
Assignee | ||
Comment 39•9 years ago
|
||
Comment on attachment 8663472 [details] [diff] [review] fixtests.patch [Approval Request Comment] Regression caused by (bug #): 1195480 User impact if declined: test failures Testing completed (on c-c, etc.): local mozmill Risk to taking this patch (and alternatives if risky): none
Attachment #8663472 -
Flags: approval-comm-aurora?
Comment 40•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/5904e4edfc3d
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 44.0
Comment 41•9 years ago
|
||
Comment on attachment 8663472 [details] [diff] [review] fixtests.patch http://hg.mozilla.org/releases/comm-aurora/rev/4829f1271bde
Attachment #8663472 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Updated•9 years ago
|
status-thunderbird42:
--- → unaffected
status-thunderbird43:
--- → fixed
status-thunderbird44:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•