Closed Bug 1192696 Opened 9 years ago Closed 9 years ago

Enable custom columns to be Grouped By Sort - backend

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 43.0

People

(Reporter: alta88, Assigned: alta88)

References

Details

Attachments

(2 files, 3 obsolete files)

It's unclear if this ever worked, or regressed by bug 481824, and the View-SortBy menuitem was finally disabled for custom columns.  The problem is that in nsMsgGroupView the view is being built and the test for a custom column handler will always fail, as there has to be a gDBView for the handler to be registered in the first place.  Some bad if logic ensures the next case condition is used, giving an illusion of semi working while in fact the rows array is corrupted.

This small patch fixes grouped view for custom columns in the backend.  It does not restore the menuitem yet because sort direction changes are broken (for all Grouped By columns, not just custom).  I will have a followup bug and patch for that once this one lands.

To test this patch, pick a custom column and sort direction, then top.opener.MsgGroupBySort(); in the console or equivalent.  The fix has been tested with a normal folder, a virtual single folder, and virtual multifolders backing the view. (Don't attempt to sort or column click in the grouped view, that's part 2).
Attached patch groupByCustColumn.patch (obsolete) — Splinter Review
Attachment #8645542 - Flags: review?(rkent)
Blocks: 1192838
asuth, i know it's a long time, but do you recall why the multifolder search xfvf subclass has methods for Get/Set CurCustomColumn like this, rather than using msgDBView methods:
http://hg.mozilla.org/comm-central/diff/986be3aa6df3/mailnews/base/src/nsMsgSearchDBView.cpp

the problem is that by skipping the db, custom column sorts (and anything else custcol) are lost merely by switching folders or tabs where the xfvf view is rebuilt.  i'm inclined to remove them in favor of msgDBView.
Flags: needinfo?(bugmail)
I think the answer is probably in the comment at http://hg.mozilla.org/comm-central/rev/986be3aa6df3#l8.138 although it's dealing with a slightly different case:
   8.138 +  // We do not have an m_db, so the default behavior (in nsMsgDBView) is not
   8.139 +  //  what we want (it will crash).  We just want someone to enumerate the
   8.140 +  //  headers that we already have.  Conveniently, nsMsgDBView already knows
   8.141 +  //  how to do this with its view enumerator, so we just use that.

Elaborating on that, gloda has a "synthetic" dbview implementation at https://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/dbview.js that works by telling a completely ephemeral search view things onSearchHit() repeatedly.  At the time I wrote that patch, I probably didn't understand the potential ramifications on (persisted) XFVF folder implementations since they subclass nsMsgSearchDBView which itself was used for ephemeral searches.

I think the test logic at https://dxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_nsMsgDBView.js#260 matches up with that.  There seems to be a custom column in the test, so probably if you were to naively back out the changes I made there, we might see m_db crashes again.  The actual use case would be to use gloda's faceted search and do the "view in list" and do something to cause the custom column state to be persisted and verify it doesn't crash.

Ah, and I should have checked bug 474701 a bit more.  I think https://bugzilla.mozilla.org/show_bug.cgi?id=474701#c6 agrees with me.

As long as you make it so the custom column works in (ephemeral) search views without immediately crashing, I think any solution that gets the XFVF persistence working would be fine.  Be it moving the logic back up and adding more guards, or having the XFVF implementation implement those methods again on its own explicitly calling the desired superclass.  I very much do not remember the subclassing/etc. going on, but I suspect you have an excellent idea of how to make that work!
Flags: needinfo?(bugmail)
Er, to clarify, if there is no m_db, the state would need to be persisted on the object.  Or things normalized so that there's just a fake in-memory-only nsIDBFolderInfo.  It's possible we cleaned that up later and so maybe there is some affordance along those lines already?
Thanks for the comments. It makes sense there is no m_db for the ephemeral search views (search messages dialog, gloda) but an xfvf view necessarily must have an m_db, as it must open the .msf to get the search string and even exist in folderpane, and indeed various other persistence items do get saved.

(In reply to Andrew Sutherland [:asuth] from comment #3)
> As long as you make it so the custom column works in (ephemeral) search
> views without immediately crashing, I think any solution that gets the XFVF
> persistence working would be fine.  Be it moving the logic back up and
> adding more guards, or having the XFVF implementation implement those
> methods again on its own explicitly calling the desired superclass.  I very
> much do not remember the subclassing/etc. going on, but I suspect you have
> an excellent idea of how to make that work!

Yes, what needs to happen is the latter, to leave [Get|Set]CurCustomColumn in the nsMsgSearchDBView class (to support custom columns) and give nsMsgXFVirtualFolderDBView its own methods to call nsMsgDBView, whose methods test for m_db anyway so crashiness there is odd.  I wonder if there are other such subclassing quirks in xfvf view due to its inheritance model.
Attached patch groupByCustomColumn1b.patch (obsolete) — Splinter Review
Actually, the former is cleaner and follows well worn precedent for such sort info; this is fairly simple asuth, if you don't mind a review, otherwise please direct to rkent.

Note that removal of nullptr in nsMsgQuickSearchDBView enables persistence of viewFlags. Saved virtual single folder and quickfilter views are fine.
Attachment #8648390 - Flags: review?(bugmail)
Comment on attachment 8648390 [details] [diff] [review]
groupByCustomColumn1b.patch

This looks good to me and seems like a very nice cleanup in general.  However, since I'm no longer a peer for any of this code, and you aren't either to be able to defer review to me, :rkent or :jcranmer or someone who can defer review or perform review themselves is needed.
Attachment #8648390 - Flags: review?(rkent)
Attachment #8648390 - Flags: review?(bugmail)
Attachment #8648390 - Flags: review+
Attached patch groupByCustomColumn1b.patch (obsolete) — Splinter Review
restore nulling m_viewFolder for single virtual folder, it's used elsewhere to actually determine that; persistence will be done elsewhere for that case.
Attachment #8648390 - Attachment is obsolete: true
Attachment #8648390 - Flags: review?(rkent)
Attachment #8649006 - Flags: review?(rkent)
I'll try to start looking at this now, which is challenging as this is not code I work with very frequently.

In testing with both patches loaded, I'm having difficulties getting grouped sort to work at all. STR:

1) Setup (without these patches) a grouped-by sorted view by date, then restart. After restart, grouped-by sort appears correctly.
2) Change sort to Subject (which resets the type to threaded)
3) Change sort type to grouped-by

Without the patches, the grouped-by subject view appears. With these patches, it does not. (This is a test profile that also has FiltaQuilla loaded that has some custom columns displayed, though I am not using those in the sort here.)

Am I missing something? Can you also see this?

I'll try to dive into the code so that I can understand what is needed for the review next.
Hmm, with your exact STR, it works for me with the patches.  On groupby, restart, change column, groupby, restart, change column, groupby it all works for the non custom col groupable sorts.  I'm using junkquilla, attachment counts (http://attachmentcount.mozdev.org/), use bcc (https://addons.mozilla.org/en-US/thunderbird/addon/use-bcc-instead/) and some others for custom column tests.  It works, persists on restart, persists on folder change for cust cols on a normal and multifolder views; single folder virtual persist is being done in the related bug.

I would note that few extensions handle grouped by sort for custom columns (mine included, the header dummy row sort string isn't for presentation) and throw a lot of treeview errors.

Neither patch code path should invoked unless a column type is byCustom and both are relatively trivial, so perhaps it's the apply or profile..?
OK sorry, I was testing with an older version (since trunk is not building) and I was confused because it was missing your previous patch about the subject line. I've updated and the issue is gone. I'll continuing looking at the patches.
Comment on attachment 8645542 [details] [diff] [review]
groupByCustColumn.patch

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

feedback+ because I accept the intent and overall direction of the patch, but would like to see some changes.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +216,5 @@
>  notFlagged=Not Starred
>  groupFlagged=Starred
>  
> +#Grouped by header row string for messages with no value for the sort attribute.
> +noGroupedValue=[No value]

If I understand correctly, this is really meant to be more of an error message for the addon author, rather than an expected user string. Perhaps we could just use symbols here instead to spare the translation burden, say just *

But I don't feel strongly about this. If you use * you could hardwire that in the code instead of adding a string here.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +6,5 @@
>  #include "msgCore.h"
>  #include "prmem.h"
>  #include "nsArrayUtils.h"
>  #include "nsIMsgCustomColumnHandler.h"
> +#include "nsMsgGroupView.h"

It's a source of unneeded complexity when a super class like this one needs a reference to one of its subclasses. Please remove this. (I tried compiling without it and had no problems, why do you need it?)

@@ +1839,5 @@
>            sortInfo.mCustomColumnName.Equals(column))
>        sortInfo.mColHandler = handler;
>    }
> +
> +  // If we're in the grouped by custom column view and the desired custom

Although the code already has some behavior specific to subclasses built into the base class, generally it is good to avoid that. Can you move this code to an override of AddColumnHandler in nsMsgGroupView.cpp since it is group view behavior?

@@ +1933,5 @@
>    NS_IF_ADDREF(*aHandler = GetColumnHandler(column.get()));
>    return (*aHandler) ? NS_OK : NS_ERROR_FAILURE;
>  }
>  
> +nsresult nsMsgDBView::RebuildView(nsMsgViewFlagsTypeValue viewFlags)

Since this is functionality that is only needed by the group view, please just make this an additional local method of nsMsgGroupView and remove it from nsMsgDBView.

Or did you have some plan to require this in other views?

::: mailnews/base/src/nsMsgDBView.h
@@ +455,5 @@
>    nsIMsgCustomColumnHandler* GetColumnHandler(const char16_t*);
>    nsIMsgCustomColumnHandler* GetCurColumnHandlerFromDBInfo();
>  
> +  // To be overridden by nsMsgGroupView::RebuildView.
> +  virtual nsresult RebuildView(nsMsgViewFlagsTypeValue viewFlags);

Per the above comment, this will end up as a non-virtual method of nsMsgGroupView

::: mailnews/base/src/nsMsgGroupView.cpp
@@ +849,5 @@
>            aValue = hashKey;
>            break;
>          case nsMsgViewSortType::byCustom:
>          {
> +          nsIMsgCustomColumnHandler* colHandler = GetCurColumnHandlerFromDBInfo();

This was originally split over two lines because otherwise it exceeds the 80 column limit, so you need to keep it split. I am not a big fan of the 80 column limit, but that is the agreed standard that we have to keep in new code.

@@ +855,2 @@
>              rv = colHandler->GetSortStringForRow(msgHdr.get(), aValue);
> +          if (aValue.IsEmpty()) {

I have mixed feelings about the sort string usage. In the past, it was not displayed, so addon authors (including myself) may have used user-unfriendly strings that were designed to sort in the correct order. You are just following the previous practice, but if this never worked before the previous practice is not really relevant.

But in the interest of avoiding new complexity, let's accept this, but please add some documentation to nsIMsgCustomColumnHandler that mentions how this is used (that is both to determine sort order, as well as the header value in group sort).

::: mailnews/base/src/nsMsgGroupView.h
@@ +55,5 @@
>                                    uint32_t *pFlags = NULL) override;
>  
> +  // Returns true if we are grouped by a sort attribute that uses a dummy row.
> +  bool GroupViewUsesDummyRow();
> +  virtual nsresult RebuildView(nsMsgViewFlagsTypeValue newFlags) override;

This will be non-virtual after the suggested changes to only define this here.
Attachment #8645542 - Flags: review?(rkent) → feedback+
(In reply to Kent James (:rkent) from comment #12)
> Comment on attachment 8645542 [details] [diff] [review]
> groupByCustColumn.patch
> 
> Review of attachment 8645542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> feedback+ because I accept the intent and overall direction of the patch,
> but would like to see some changes.
> 
> ::: mail/locales/en-US/chrome/messenger/messenger.properties
> @@ +216,5 @@
> >  notFlagged=Not Starred
> >  groupFlagged=Starred
> >  
> > +#Grouped by header row string for messages with no value for the sort attribute.
> > +noGroupedValue=[No value]
> 
> If I understand correctly, this is really meant to be more of an error
> message for the addon author, rather than an expected user string. Perhaps
> we could just use symbols here instead to spare the translation burden, say
> just *
> 
> But I don't feel strongly about this. If you use * you could hardwire that
> in the code instead of adding a string here.
> 

It was really so a user wouldn't see emptiness if the author didn't consider handling this view and data possibility. But * (10) is better than just (10) and is fine and simpler.

> @@ +855,2 @@
> >              rv = colHandler->GetSortStringForRow(msgHdr.get(), aValue);
> > +          if (aValue.IsEmpty()) {
> 
> I have mixed feelings about the sort string usage. In the past, it was not
> displayed, so addon authors (including myself) may have used user-unfriendly
> strings that were designed to sort in the correct order. You are just
> following the previous practice, but if this never worked before the
> previous practice is not really relevant.
> 
> But in the interest of avoiding new complexity, let's accept this, but
> please add some documentation to nsIMsgCustomColumnHandler that mentions how
> this is used (that is both to determine sort order, as well as the header
> value in group sort).
> 

Right.  I considered using GetCellText here, the addon's function would need to determine if it's a header row: getFlagsAt(row) & MSG_VIEW_FLAG_DUMMY, but then creating the buckets *could* be more complex (and duplicate creating buckets) than making a pretty GetSortStringForRow.

Rest is fixed, builds with other patches unchanged up to the latest wip in bug 1192838.
Attachment #8645542 - Attachment is obsolete: true
Attachment #8650270 - Flags: review?
Attachment #8650270 - Flags: review? → review?(rkent)
Assignee: nobody → alta88
Comment on attachment 8650270 [details] [diff] [review]
groupByCustColumn.patch

OK looks good. I'll keep looking at the other patches as well, but it takes me hours each so please be patient.
Attachment #8650270 - Flags: review?(rkent) → review+
np, thanks for reviewing. There are multitudes of variations in all this but the patches are an attempt to be modular about it.
tweaked for setter to persist but check for viewFolder first.
Attachment #8649006 - Attachment is obsolete: true
Attachment #8649006 - Flags: review?(rkent)
Attachment #8651827 - Flags: review?(rkent)
Comment on attachment 8651827 [details] [diff] [review]
groupByCustomColumn1b.patch

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

OK looks good to me. One nit (the newly misnamed GetCurColumnHandlerFromDBInfo) please rename this (including of course its multiple calls).

To test this I had to load some of your other patches, but it seems to work fine with them and makes sense.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +1869,5 @@
>    return NS_ERROR_FAILURE; //can't remove a column that isn't currently custom handled
>  }
>  
>  //TODO: NS_ENSURE_SUCCESS
>  nsIMsgCustomColumnHandler* nsMsgDBView::GetCurColumnHandlerFromDBInfo()

This method is now misnamed, as you no longer get this from DBFolderInfo. Maybe just rename to GetCurColumnHandler?
Attachment #8651827 - Flags: review?(rkent) → review+
Thanks, I will add the rename in bug 1195480 so as not to bitrot the patches already there.
Keywords: checkin-needed
(In reply to alta88 from comment #18)
> Thanks, I will add the rename in bug 1195480 so as not to bitrot the patches
> already there.

Yes that is fine.
https://hg.mozilla.org/comm-central/rev/107e7db514436ae6659e5c9b1f42f5fb9231deae
Bug 1192696 - Enable custom columns to be Grouped By Sort - backend. r=rkent

https://hg.mozilla.org/comm-central/rev/657ad2a8f8f8b51703eeddb82e757cf04eedbee4
Bug 1192696 - Enable custom columns to be Grouped By Sort - backend, part2: make [G|S]etCustomColumn() operate on nsMsgDBView base class value only, persist only if m_viewFolder exists, remove from nsMsgSearchDBView. r=rkent
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
(In reply to alta88 from comment #18)
> Thanks, I will add the rename in bug 1195480 so as not to bitrot the patches
> already there.

++++
It's nice to see someone giving love to Grouped bugs
You need to log in before you can comment on or make changes to this bug.