Closed Bug 1719093 Opened 1 year ago Closed 1 year ago

Extend MailTab to allow getting / setting the threaded / group by state of views

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement)

enhancement

Tracking

(thunderbird_esr78 wontfix, thunderbird91 fixed)

RESOLVED FIXED
91 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird91 --- fixed

People

(Reporter: standard8, Assigned: TbSync)

Details

Attachments

(2 files)

Currently MailTab allows getting and setting of the sort orders, but does not allow getting/setting of the theaded / group by state of views.

Since the two are fairly linked, I think it would be reasonable to have both available.

Proposing to add "groupBy" with values "none", "thread" and "sort".

So the grouped view is a bit special, and different from the threaded/unthreaded view. The api should allow and setting "threaded", "unthreaded", "groupedBy" commands. The threading state should also be available. The "groupedBy" command would require a "sortType" argument (a number of sort types are invalid for grouped view). The "groupedBy" command should also handle custom columns.

A good stress test of the api would be to do sort/secondary sort using 2 custom columns. See Bug 1192696, Bug 1192838, Bug 1195480 for reference.

There are some things missing for api completeness in exposing gDBview properies.

  • Expanded/collapsed thread state (folder level) and updating it
  • Expanded/collapsed thread state of a message thread parent and toggling it
  • For sortType of custom, then gDBView.curCustomColumn is necessary
  • The secondary sort properties should be available:
  • gDBView.secondarySortType,gDBView.secondarySortOrder, gDBView.secondaryCustomColumn
  • A command that takes primary sort order/type and secondary order/type; call gFolderDisplay.view.sort()

(In reply to alta88 from comment #2)

So the grouped view is a bit special, and different from the threaded/unthreaded view. The api should allow and setting "threaded", "unthreaded", "groupedBy" commands.

As per comment 1, John and I came up with calling the parameter groupBy, because that's essentially what it is doing in grouping the messages - none is unthreaded, thread is obvious, and sort is what you call "groupedBy", but it makes sense because you have groupBy: "sort" which reads nicely. We could possibly make it groupBy: "sortType" to directly reference the other parameter in the API, but I'm not convinced it is necessary.

The threading state should also be available.

I don't know what you mean by this. You cannot have a combination of unthreaded/threaded/grouped from my testing of the UI.

The "groupedBy" command would require a "sortType" argument (a number of sort types are invalid for grouped view). The "groupedBy" command should also handle custom columns.

We already have sortType as part of the existing API, so we don't need to do anything extra here, apart from to check if it is valid or not (or even just leave that up to the core code and make sure we return a valid error message).

A good stress test of the api would be to do sort/secondary sort using 2 custom columns. See Bug 1192696, Bug 1192838, Bug 1195480 for reference.

I think that's probably worth handling in a follow-up. If the existing API needs changing because it doesn't support it, then that's an existing issue. Just adding the threaded state doesn't change if it works or not.

There are some things missing for api completeness in exposing gDBview properies.

  • Expanded/collapsed thread state (folder level) and updating it
  • Expanded/collapsed thread state of a message thread parent and toggling it
  • For sortType of custom, then gDBView.curCustomColumn is necessary
  • The secondary sort properties should be available:
  • gDBView.secondarySortType,gDBView.secondarySortOrder, gDBView.secondaryCustomColumn
  • A command that takes primary sort order/type and secondary order/type; call gFolderDisplay.view.sort()

I think they would be fine as separate enhancements, though I'm not convinced WebExtensions need to implement all of them, unless there's a demonstrated need.

(In reply to Mark Banner (:standard8) from comment #3)

(In reply to alta88 from comment #2)

So the grouped view is a bit special, and different from the threaded/unthreaded view. The api should allow and setting "threaded", "unthreaded", "groupedBy" commands.

As per comment 1, John and I came up with calling the parameter groupBy, because that's essentially what it is doing in grouping the messages - none is unthreaded, thread is obvious, and sort is what you call "groupedBy", but it makes sense because you have groupBy: "sort" which reads nicely. We could possibly make it groupBy: "sortType" to directly reference the other parameter in the API, but I'm not convinced it is necessary.

It's confusing to those familiar with internal syntax to change names for the we apis, so I really think it's important to retain that. Currently, if you test gFolderDisplay.view.showGroupedBySort and it's true, then gFolderDisplay.view.showThreaded will by definition be false. So having a groupBy parameter with any threading args isn't right. If there is a groupBy parameter, it should also require a sortType argument. For custom columns, it requires the name of the column rather that the built in numeric Ci.nsMsgViewSortType values.

The threading state should also be available.

I don't know what you mean by this. You cannot have a combination of unthreaded/threaded/grouped from my testing of the UI.

I mean there should be a thread parameter with true/false arguments to set thread state. Such a command will automatically switch out of groupedBy view.

There should be something like isThreaded and isGroupedBy to get those states.

The "groupedBy" command would require a "sortType" argument (a number of sort types are invalid for grouped view). The "groupedBy" command should also handle custom columns.

We already have sortType as part of the existing API, so we don't need to do anything extra here, apart from to check if it is valid or not (or even just leave that up to the core code and make sure we return a valid error message).

What if sortType is set to custom? Where in the api do you get which custom column?

A good stress test of the api would be to do sort/secondary sort using 2 custom columns. See Bug 1192696, Bug 1192838, Bug 1195480 for reference.

I think that's probably worth handling in a follow-up. If the existing API needs changing because it doesn't support it, then that's an existing issue. Just adding the threaded state doesn't change if it works or not.

Not sure what this means. My comment has to do with making sure the api can set sort correctly, both primary and secondary. Not anything to do with threaded state.

There are some things missing for api completeness in exposing gDBview properies.

  • Expanded/collapsed thread state (folder level) and updating it
  • Expanded/collapsed thread state of a message thread parent and toggling it
  • For sortType of custom, then gDBView.curCustomColumn is necessary
  • The secondary sort properties should be available:
  • gDBView.secondarySortType,gDBView.secondarySortOrder, gDBView.secondaryCustomColumn
  • A command that takes primary sort order/type and secondary order/type; call gFolderDisplay.view.sort()

I think they would be fine as separate enhancements, though I'm not convinced WebExtensions need to implement all of them, unless there's a demonstrated need.

My extensions would need them. It's unfortunate that Tb hasn't implemented a visual indicator for secondary sort and I have to do it in MessagePreview. In another extension, I implemented a very clear secondary sort UI.

(In reply to alta88 from comment #4)

It's confusing to those familiar with internal syntax to change names for the we apis, so I really think it's important to retain that.

I think it is wrong to limit ourselves to the internal syntax. Firstly, I'd argue that the internal syntax is really bad here, secondly, the internal syntax could change in future, and then it would be different anyway.

Currently, if you test gFolderDisplay.view.showGroupedBySort and it's true, then gFolderDisplay.view.showThreaded will by definition be false. So having a groupBy parameter with any threading args isn't right.

It is a slightly different UI style, but you are still grouping by thread, so it does make sense in my opinion.

If there is a groupBy parameter, it should also require a sortType argument. For custom columns, it requires the name of the column rather that the built in numeric Ci.nsMsgViewSortType values.

Yes, we can require that sortType is specified in addition to groupBy in the update() function.

I mean there should be a thread parameter with true/false arguments to set thread state. Such a command will automatically switch out of groupedBy view.

There should be something like isThreaded and isGroupedBy to get those states.

So you can have either isThreaded or isGroupedBy individually true, but not true at the same time? Why not just make it simpler then, and have the tri-state variable as suggested?

The "groupedBy" command would require a "sortType" argument (a number of sort types are invalid for grouped view). The "groupedBy" command should also handle custom columns.

We already have sortType as part of the existing API, so we don't need to do anything extra here, apart from to check if it is valid or not (or even just leave that up to the core code and make sure we return a valid error message).

What if sortType is set to custom? Where in the api do you get which custom column?

We could add a list of APIs or something, I don't think we should fix that here.

A good stress test of the api would be to do sort/secondary sort using 2 custom columns. See Bug 1192696, Bug 1192838, Bug 1195480 for reference.

I think that's probably worth handling in a follow-up. If the existing API needs changing because it doesn't support it, then that's an existing issue. Just adding the threaded state doesn't change if it works or not.

Not sure what this means. My comment has to do with making sure the api can set sort correctly, both primary and secondary. Not anything to do with threaded state.

I meant we should not fix primary/secondary sort issues here. This bug does not affect the status-quo with respect to primary/secondary sort. That's why I'm suggesting a follow-up.

There are some things missing for api completeness in exposing gDBview properies.

  • Expanded/collapsed thread state (folder level) and updating it
  • Expanded/collapsed thread state of a message thread parent and toggling it
  • For sortType of custom, then gDBView.curCustomColumn is necessary
  • The secondary sort properties should be available:
  • gDBView.secondarySortType,gDBView.secondarySortOrder, gDBView.secondaryCustomColumn
  • A command that takes primary sort order/type and secondary order/type; call gFolderDisplay.view.sort()

I think they would be fine as separate enhancements, though I'm not convinced WebExtensions need to implement all of them, unless there's a demonstrated need.

My extensions would need them. It's unfortunate that Tb hasn't implemented a visual indicator for secondary sort and I have to do it in MessagePreview. In another extension, I implemented a very clear secondary sort UI.

I suggest handling these in one or more follow-ups. Some of these would be different APIs, and some will possibly need thinking about a little more.

(In reply to Mark Banner (:standard8) from comment #5)

(In reply to alta88 from comment #4)

It's confusing to those familiar with internal syntax to change names for the we apis, so I really think it's important to retain that.

I think it is wrong to limit ourselves to the internal syntax. Firstly, I'd argue that the internal syntax is really bad here, secondly, the internal syntax could change in future, and then it would be different anyway.

Not sure what's really bad. The existing syntax isn't going to change, I'm the last person to make any mildly significant changes to the view code. And very unsure that nsITree for threadpane views will ever be replaced, with full feature/perf equivalence, sorry.

If there is a groupBy parameter, it should also require a sortType argument. For custom columns, it requires the name of the column rather that the built in numeric Ci.nsMsgViewSortType values.

Yes, we can require that sortType is specified in addition to groupBy in the update() function.

I'm saying you also must require customColumnName, eg, if sortType is custom. Otherwise the api is not complete.

I mean there should be a thread parameter with true/false arguments to set thread state. Such a command will automatically switch out of groupedBy view.

There should be something like isThreaded and isGroupedBy to get those states.

So you can have either isThreaded or isGroupedBy individually true, but not true at the same time? Why not just make it simpler then, and have the tri-state variable as suggested?

Well, then you return some string token instead of bools. Less optimal but fine; then calling it groupBy is incorrect. It should be something likeviewType.

There are some things missing for api completeness in exposing gDBview properies.

  • Expanded/collapsed thread state (folder level) and updating it
  • Expanded/collapsed thread state of a message thread parent and toggling it
  • For sortType of custom, then gDBView.curCustomColumn is necessary
  • The secondary sort properties should be available:
  • gDBView.secondarySortType,gDBView.secondarySortOrder, gDBView.secondaryCustomColumn
  • A command that takes primary sort order/type and secondary order/type; call gFolderDisplay.view.sort()

I think they would be fine as separate enhancements, though I'm not convinced WebExtensions need to implement all of them, unless there's a demonstrated need.

My extensions would need them. It's unfortunate that Tb hasn't implemented a visual indicator for secondary sort and I have to do it in MessagePreview. In another extension, I implemented a very clear secondary sort UI.

I suggest handling these in one or more follow-ups. Some of these would be different APIs, and some will possibly need thinking about a little more.

That's fine, I just want to make sure these additions to ensure a rich api, worthy of replacing experiments, are documented.

(In reply to alta88 from comment #6)

If there is a groupBy parameter, it should also require a sortType argument. For custom columns, it requires the name of the column rather that the built in numeric Ci.nsMsgViewSortType values.

Yes, we can require that sortType is specified in addition to groupBy in the update() function.

I'm saying you also must require customColumnName, eg, if sortType is custom. Otherwise the api is not complete.

Okay, that makes sense. As far as I can tell this is also an issue for setting the sort to custom today, so I've filed bug 1719796 to handle that.

I mean there should be a thread parameter with true/false arguments to set thread state. Such a command will automatically switch out of groupedBy view.

There should be something like isThreaded and isGroupedBy to get those states.

So you can have either isThreaded or isGroupedBy individually true, but not true at the same time? Why not just make it simpler then, and have the tri-state variable as suggested?

Well, then you return some string token instead of bools.

That's what we're suggesting.

Less optimal but fine; then calling it groupBy is incorrect. It should be something likeviewType.

I do not see why it is less optimal. I would argue that having two different attributes which are interlocked but related to the same thing gives more cognitive load to a developer than when they are handled within the same attribute.

I still think groupBy would work well, for the reasons I stated above. However, I would also be happy to go with viewType, though maybe not quite as clear.

If we go with view type, then I think we should use the names for list, threaded and groupBy - I would not be against using unthreaded instead of list, but since that state is really not threaded and not groupBy, list seems more appropriate.

So what is the plan? If I have to wait till the column bug has been fixed, this is not gonna make it in TB91.

Can we design the API in such a way, that we can have the three state viewType property and add support for custom columns later?

(In reply to John Bieling (:TbSync) from comment #8)

Can we design the API in such a way, that we can have the three state viewType property and add support for custom columns later?

I think we can add the viewType property. The existing API doesn't properly support custom columns already, so this is not making it any worse.

If we go with view type, then I think we should use the names for list, threaded and groupBy - I would not be against using unthreaded instead of list, but since that state is really not threaded and not groupBy, list seems more appropriate.

Yes, viewType should be a getter/setter type thing. Setting it, currently, for list, threaded and groupBy is great.

Thanks for filing the custom column bug.

So the api would look like this:

update([tabId], {viewType:"list"})
update([tabId], {viewType:"list", sortType:"author", sortOrder: "ascending"})
update([tabId], {viewType:"threaded})
update([tabId], {viewType:"threaded", sortType:"date", sortOrder: "ascending"})
update([tabId], {viewType:"groupBy")
update([tabId], {viewType:"groupBy", sortType:"unread"})
update([tabId], {viewType:"groupBy", sortType:"custom", customColumnName:"messagePreviewCol"})

Where sortType is absent, the current sortType/sortOrder would be used. Eventually secondarySortType andsecondarySortOrder should be included.

Hm. The implementation is simple, but I am unhappy with the names.

viewType: "groupBy"

does not make much sense to me, because the entire thing is about grouping. I therefore still tend to name the property groupBy with the following values:

  • none
  • thread
  • sortType

The name viewType is good as well, but I think it needs different values which explain the grouping nature, like

  • normal
  • groupByThread
  • groupBySortType

What do you think? Geoff?

Flags: needinfo?(geoff)
Assignee: nobody → john
Status: NEW → ASSIGNED
Target Milestone: --- → 91 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1e708bb5f36d
Add support for message grouping to mailTabs API. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

Your successor will spend cognitive load trying to understand why ancient and omnipresent syntax in the backend was changed for this api. And "normal" could not be a worse descriptor; it implies you know what normal is, imparts no meaning to the actual view state, must be documented explicitly.

+              nativeTab.folderDisplay.view.showGroupedBySort = false;

Why is this necessary; setting the correct view flags is already done in the showThreaded setter.

+              nativeTab.folderDisplay.view.showGroupedBySort = false;

Why is this necessary; setting the correct view flags is already done in the showThreaded setter.

Test failed without it.

Then the test is wrong. It would be important to correctly test that the setter is updating all flags on a view change, as intended, rather than adding such hacks to the code.

One last thing: the View->Sort by menulist UI contains Threaded, Unthreaded, and Grouped By Sort options. No "normal" or "groupedbythread".

@alta88: You have a valid point there regarding the setters. I switched to use

nativeTab.folderDisplay.view.showUnthreaded = true

in https://phabricator.services.mozilla.com/D119665. Are you ok with that?

Flags: needinfo?(geoff)
Attachment #9230787 - Attachment description: WIP: Bug 1719093 - Improve setting/testing normal viewtype. r=mkmelin → WIP: Bug 1719093 - Improve setting/testing normal viewtype. r=darktrojan
Attachment #9230787 - Attachment description: WIP: Bug 1719093 - Improve setting/testing normal viewtype. r=darktrojan → Bug 1719093 - Improve setting/testing normal viewtype. r=darktrojan
Attachment #9230787 - Attachment description: Bug 1719093 - Improve setting/testing normal viewtype. r=darktrojan → Bug 1719093 - Improve setting/testing normal viewType. r=darktrojan

(In reply to John Bieling (:TbSync) from comment #19)

@alta88: You have a valid point there regarding the setters. I switched to use

nativeTab.folderDisplay.view.showUnthreaded = true

in https://phabricator.services.mozilla.com/D119665. Are you ok with that?

Yes.

Attachment #9230787 - Attachment description: Bug 1719093 - Improve setting/testing normal viewType. r=darktrojan → Bug 1719093 - Improve setting/testing normal viewtype. r=mkmelin
Attachment #9230787 - Attachment description: Bug 1719093 - Improve setting/testing normal viewtype. r=mkmelin → Bug 1719093 - Rename normal viewType to ungrouped and improve its setting/testing. r=darktrojan
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/dcdfee7c4389
Rename normal viewType to ungrouped and improve its setting/testing. r=darktrojan

Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED

Comment on attachment 9230787 [details]
Bug 1719093 - Rename normal viewType to ungrouped and improve its setting/testing. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Unclear property name "normal" instead of "ungrouped" (and backward incompatible)

Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
Low.

Attachment #9230787 - Flags: approval-comm-beta?

Comment on attachment 9230787 [details]
Bug 1719093 - Rename normal viewType to ungrouped and improve its setting/testing. r=darktrojan

[Triage Comment]
Approved for beta

Attachment #9230787 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.