1671557 - Some APIs in `nsITableEditor` will be changed by bug 1671556

Status: RESOLVED FIXED

(Thunderbird :: Upstream Synchronization, task)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See bug 1671556 comment 0 for the detail. I'll get rid of the stateful API to make table editor simpler and create new API which is easier to use. The new design will be posted here (not fixed yet in my brain).

Comment 1

[Magnus Melin [:mkmelin]]

Changes needed for the stuff on autoland bug 1671556 comment 15:

• https://searchfox.org/comm-central/search?q=getFirstSelectedCell&path=mail&case=false&regexp=false - https://hg.mozilla.org/integration/autoland/diff/585df60d18f9ec622913f0474c462f851fbcaa0a/editor/nsITableEditor.idl - no out range for editor.getFirstSelectedCell(rangeObj)
• replace nsITableEditor.getFirstSelectedCell()andnsITableEditor.getNextSelectedCell()withnsITableEditor.getSelectedCells() - https://searchfox.org/comm-central/search?q=getFirstSelectedCell%7CgetNextSelectedCell&path=mail&case=false&regexp=true

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, it is. Nobody uses the selecred range for a selected cell element, and it can know really easy, the range is same as the result of Range.selectNode(). And the new API simply returns array of selected cells at once. It ignores selection ranges whose index is 2 or more if a range does not select only one cell element. And different from the old API, it does not throw exception unless editor has already destroyed.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1671557_editor_changes.patch

I think this covers it.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9183632 [details] [diff] [review]
bug1671557_editor_changes.patch

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

Thank you for the quick fix. Getting rid of the stateful API makes `HTMLEditor` much simpler.

::: mail/components/compose/content/dialogs/EdTableProps.js
@@ +1319,4 @@
>      return;
>    }
>  
>    if (gSelectedCellCount == 1) {

In strictly speaking, this is not safe. If `selectedCells.length` is not 1, the selection and/or the DOM tree has been changed after `gSelectedCellCount` is updated.

::: mail/components/compose/content/editor.js
@@ +2302,5 @@
> +  // We have at least one row
> +  rows++;
> +
> +  var lastIndex = rowObj.value;
> +  for (let cell of editor.getSelectedCells()) {

I'm not familiar with JS though, is this `getSelectedCells()` called once?

@@ +2333,5 @@
> +  // We have at least one column
> +  columns++;
> +
> +  var lastIndex = colObj.value;
> +  for (let cell of editor.getSelectedCells()) {

Ditto.

Comment 5

[Khushil Mistry [:khushil324]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

In strictly speaking, this is not safe. If selectedCells.length is not 1,
the selection and/or the DOM tree has been changed after
gSelectedCellCount is updated.

gSelectedCellCount is updated every time we select anything in the table. But I guess we can use editor.getSelectedCells().length now and remove the usage of the gSelectedCellCount.

I'm not familiar with JS though, is this getSelectedCells() called once?

Yes, it gets called only once.

Comment 6

[Khushil Mistry [:khushil324]]

Comment on attachment 9183632 [details] [diff] [review]
bug1671557_editor_changes.patch

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

All the changes look good to me. I have sent it for a try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=77d838f347c5cc7c4a85382e379dd1bdba96703b
Check once on ``gSelectedCellCount``.

Comment 7

[Magnus Melin [:mkmelin]]

I'll change this occurrence to use getSelectedCells().length instead of gSelectedCellCount, but leave other places using it since doesn't look like I can get rid of it very trivially atm.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd200251aa68
port editor.getFirstSelectedCell and editor.getNextSelectedCell changes from bug 1671556. r=khushil DONTBUILD

Comment 9

[Magnus Melin [:mkmelin]]

Reopening as I suppose there is more coming.

Comment 10

[Magnus Melin [:mkmelin]]

Looks like we're done.