Bug 151840 - API: Text Table Cursor has no notion of row and columns, and no sane way to loop through the selection
Summary: API: Text Table Cursor has no notion of row and columns, and no sane way to l...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium enhancement
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: Writer-Tables-Select
  Show dependency treegraph
 
Reported: 2022-10-31 09:45 UTC by Lionel Elie Mamane
Modified: 2023-04-22 03:55 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
example 1 of B1:D3 (9.68 KB, image/png)
2022-11-01 14:12 UTC, Lionel Elie Mamane
Details
example 2 of B1:D3 (8.67 KB, image/png)
2022-11-01 14:12 UTC, Lionel Elie Mamane
Details
example 3 of B1:D3 (8.44 KB, image/png)
2022-11-01 14:13 UTC, Lionel Elie Mamane
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2022-10-31 09:45:12 UTC
Description:
When a bunch of cells are selected in Writer, ThisComponent.getCurrentSelection() returns a css:text:TextTableCursor. That provides no sane way to traverse (loop through) the selection, nor to get the boundaries of the selection, etc.

There is a getRangeName() and a gotoCellByName(...) which allow to do it, by... manually parsing and constructing string cell range names like "A4:C53". That is not a sane, programmer-friendly interface.

The goXXX() methods kinda make sense as a general cursor in the table, but are not useful in the context of the selection; they will happily exit the selection without any indication that they did so.

As a side-note, the choice to have goRight/goLeft silently wrap to next/previous row is maybe also not the most useful API choice. We are stuck with that for compatibility reasons, so I won't mention it any more.

Steps to Reproduce:
.

Actual Results:
.

Expected Results:
.


Reproducible: Always


User Profile Reset: No

Additional Info:
.
Comment 1 Rafael Lima 2022-10-31 23:16:38 UTC
Apparently, there's no easy way to do this. See:

https://ask.libreoffice.org/t/modifying-selected-cells-in-writer-table-through-a-macro/48076/2

@Mike, I'm CC'ing you since you are one of the respondents in the link above.

What do you think of this? Should this be a NEW or a WF ?

Are the necessary changes in the API too complex?
Comment 2 Mike Kaganski 2022-11-01 04:46:06 UTC
(In reply to Rafael Lima from comment #1)
> What do you think of this? Should this be a NEW or a WF ?

As I mentioned there, I share the frustration about the object.

> Are the necessary changes in the API too complex?

No. The question is - what is necessary? I'd say - having a TextTable property, and implementing com::sun::star::container::XEnumerationAccess to iterate cells, would possibly cover the majority of problems?
Comment 3 Lionel Elie Mamane 2022-11-01 10:22:34 UTC
(In reply to Mike Kaganski from comment #2)

> I'd say - having a TextTable property, and implementing
> com::sun::star::container::XEnumerationAccess to
> iterate cells, would possibly cover the majority of problems?

This is a table, it has a 2D structure. That structure should be accessible as such in the API. From the general cursor point of view, I'd say the bare minimum is to get the current position, and "jump" to a position, with two integers being row number and column number. Plus, since cells can be merged, need some way to know which (row, column) pairs are valid cells and which are not (because they have been merged and they not the "first" (top/left-most) cell in the merge).

The API seems to be already defined in our idl files, but not actually exposed?

css:Text:TableRows, which exposes the same css:table:XTableRows that the service css:table:TableRows exposes (but no XEnurmerationAccess, while css:table:TableRows does, so your idea to add that makes sense there at least).

css:text:CellRange exposes css:table:XCellRange, like css:table:CellRange does. One might believe this may be an API design error, since css:table:XCellRange:getCellByPosition() returns a css:table:XCell (and one might believe a text table cell cannot necessarily provide that interface), but actually the css:text:Cell service provides the css:table:XCell interface, so at least it is consistent.

To me, it looks like the API is there, we "just" need to implement it (if that is not already done?), and provide the objects that implement it where reasonable.

For example, for my initial use case which prompted me to start this discussion, it looks like if ThisComponent.getCurrentSelection() returned an object that implements css:text:CellRange I would be (mostly?) happy.
Comment 4 Rafael Lima 2022-11-01 11:27:41 UTC
I would also support the implementation of XCellRange, since it provides access by row/column.

Also, the current selection should allow to access the selected TextTable object, which is currently not possible.
Comment 5 Mike Kaganski 2022-11-01 11:53:46 UTC
(In reply to Lionel Elie Mamane from comment #3)
> This is a table, it has a 2D structure. That structure should be accessible
> as such in the API. ...

(In reply to Rafael Lima from comment #4)
> I would also support the implementation of XCellRange, since it provides
> access by row/column. ...

No, it doesn't work that way ;) It's not OK to implement whatever interfaces *could* be nice to have; there should be a reason to add anything.

For example: why do I need a TextTable property, or XEnumerationAccess.
Consider the code:

  oSel = ThisComponent.CurrentSelection
  If (HasUnoInterfaces(oSel, "com.sun.star.text.XTextTableCursor")) Then
    ' I want to get the table object where oSel is in; I can only do that
    ' using a workaround now:
    oViewCursor = ThisComponent.CurrentController.getViewCursor()
    oTab=oViewCursor.TextTable
    ' I need to iterate the selected cells, without any specific order;
    ' say, I need to format the paragraph inside...
    oCellRange = oTab.getCellRangeByName(oSel.getRangeName())
    ' ...
  End If

If I had either of the things I mentioned, the task would become easier; and additionally, the XTextTableCursor could arrive not only from selection, but also from some programmatic way, without having corresponding view cursor, which would make the task impossible.

So if you think there should be something new implemented, there should appear some specific discussion which scenario is simplified (or even becomes possible) with the new stuff, and why it is relevant.
Comment 6 Rafael Lima 2022-11-01 12:45:44 UTC
(In reply to Mike Kaganski from comment #5)
> So if you think there should be something new implemented, there should
> appear some specific discussion which scenario is simplified (or even
> becomes possible) with the new stuff, and why it is relevant.

To be honest, I'm not an expert in API design, so my arguments are from a end-user perspective only.

Initially I wish it were possible to run the following code in a Writer document with a table selected (assuming that the selection implemented XTextTable):

oSel = ThisComponent.CurrentSelection
If HasUnoInterfaces(oSel, "com.sun.star.text.XTextTable") Then
    ' For the sake of an example, print the number of rows selected
    MsgBox oSel.getRows().getCount()
End If

But maybe the code above is a bit naive.

From your suggestion, if the CurrentSelection had a TextTable property, the same could be written as:

oSel = ThisComponent.CurrentSelection
oTab = oSel.TextTable
MsgBox oTab.getRows().getCount()

Is this what you're proposing?
Comment 7 Mike Kaganski 2022-11-01 13:01:52 UTC
(In reply to Rafael Lima from comment #6)

First of all, cursor is not a table. Implementing XTextTable in the cursor would be a wrong mix of concepts.
Then, in the context of *writer's* (in general case) non-rectangular tables, "number of selected columns" makes no sense at all, and pretending that it makes sense by providing respective API would be wrong.
Then, the TextTable property would not represent the selection, but the whole table (that has *some* of its cells selected). In that view, it's unclear how much this code makes sense:

oCellRange = oSel.TextTable.getCellRangeByName(oSel.getRangeName())

because, say, oSel.TextTable could be a 3-row table with first and third rows having 5 cells, and second row having 2 cells; and oSel.getRangeName() could be "B1:D3" - what is there in 2nd row selected? Does getCellRangeByName do the same as mouse selection? In that case, enumeration access makes the most sense.
Comment 8 Lionel Elie Mamane 2022-11-01 14:08:43 UTC
(In reply to Mike Kaganski from comment #7)
> First of all, cursor is not a table. Implementing XTextTable in the cursor
> would be a wrong mix of concepts.

I technically agree with the narrow statement above, but I see no "wrong mix of concept" in that the TextTableCursor can give access to the table it is a cursor in. Which you also propose, through a TextTable property or a getTextTable() method (which Basic would "present" as a read-only TextTable pseudo-property).

However, I must stress that, as far as I can observe, css:text:TextTableCursor is returned by ThisComponent.getCurrentSelection() only when a range of cells is selected. In that case, it seems css:text:CellRange is the service that was thought out for this case, and makes sense. Whether the object returned by getCurrentSelection() implements that service, or has a property containing an object that implements that service, I can be live with either.

> Then, in the context of *writer's* (in general case) non-rectangular tables,
> "number of selected columns" makes no sense at all, and pretending that it
> makes sense by providing respective API would be wrong.

Consider this code:

dim rngName as String
rngName = oSel.getRangeName()
dim rngPoints(2) As String
rngPoints = split(rngName, ":")
dim frstCol, lastCol as Long
frstCol = convert_letters_to_number(rngPoints(0))
lastCol = convert_letters_to_number(rngPoints(1))
MsgBox(lastCol - frstCol + 1)

That is _currently_ the notion of "number of columns selected" that the cursor _already_ has. One would just provide saner/easier access to the same information. Some of the cells may be split and thus some rows in the range have more columns that the calculated value? That's just the nature of text tables, which the programmer will have to deal with.

The LibreOffice UI presents the user with a notion of column and a notion of row. *Something* happens when one right-clicks on a cell and chooses "Delete / Delete Columns". Similarly, the cursor on the edge of a table becomes an arrow, and then clicking selects what is identified, in the UI, as a row or a column.


> Then, the TextTable property would not represent the selection, but the
> whole table (that has *some* of its cells selected). In that view, it's
> unclear how much this code makes sense:
 
> oCellRange = oSel.TextTable.getCellRangeByName(oSel.getRangeName())
 
> because, say, oSel.TextTable could be a 3-row table with first and third
> rows having 5 cells, and second row having 2 cells; and oSel.getRangeName()
> could be "B1:D3" - what is there in 2nd row selected?

I believe that _is_ well-defined within LibreOffice, at least at the UI level. The definition might not be straightforward, it might not be easy to understand or immediately intuitive, but it is there. The UI doesn't let one select any set of rows, only "generalised rectangles", which can be expressed as "LD:LD" where L is letters and D is a digits. The selection is shown in this syntax in the status bar (along with the name of the table).

So to get the answer to your example, just put the mouse pointer in the B1 cell, press the mouse button, go to the D3 cell and release the mouse button. That's the "B1:D3" selection and getCellRangeByName("B1:D3") should (and I expect already does) refer to the same range.

Answering your question cannot be done, because it is underspecified, it depends on the fine structure of the table.

> Does getCellRangeByName do the same as mouse selection?

Yes, we have to be consistent. Mouse selection calls that "B1:D3", why have a different understanding of the same string?

> In that case, enumeration access makes the most sense.

It can make sense, but people will need to access the 2D structure of the table. Since text tables are "weird", they _will_ get weird corner cases if they expect to operate on arbitrary text tables. We _should_ provide them the API access to be consistent with the UI behaviour, with the same notion of column of "generalised rectangle", etc.
Comment 9 Lionel Elie Mamane 2022-11-01 14:12:17 UTC
Created attachment 183360 [details]
example 1 of B1:D3
Comment 10 Lionel Elie Mamane 2022-11-01 14:12:46 UTC
Created attachment 183361 [details]
example 2 of B1:D3
Comment 11 Lionel Elie Mamane 2022-11-01 14:13:06 UTC
Created attachment 183362 [details]
example 3 of B1:D3
Comment 12 Mike Kaganski 2022-11-01 14:21:26 UTC
(In reply to Lionel Elie Mamane from comment #8)

For all that, simple introduction of TextTable property is enough, together with existing getRangeName. If one wants to treat something as "generalised rectangle", constructing that explicitly is IMO OK. Additionally that has benefit of not requiring that all future implementations only allow contiguous cell selection.
Comment 13 Lionel Elie Mamane 2022-11-03 10:20:00 UTC
(In reply to Mike Kaganski from comment #12)
> (In reply to Lionel Elie Mamane from comment #8)
> 
> For all that, simple introduction of TextTable property is enough, together
> with existing getRangeName.

No. getRangeName()/gotoCellByName means the macro writer must parse the range name, a string, manually. This is not a sane, usable API. Something like css:table:XCellRange is much better. Clearly, the designer of the idl files intended that to be provided for text tables, since css:text:CellRange provides that interface.
Comment 14 Mike Kaganski 2022-11-03 10:55:29 UTC
(In reply to Lionel Elie Mamane from comment #13)
> No. getRangeName()/gotoCellByName means ...

Please! :) I meant "oSel.TextTable.getCellRangeByName(oSel.getRangeName())", "getCellRangeByName" being already mentioned here in the discussion, without any manual parsing.
Comment 15 Lionel Elie Mamane 2022-11-03 12:03:27 UTC
(In reply to Mike Kaganski from comment #14)
> (In reply to Lionel Elie Mamane from comment #13)
> > No. getRangeName()/gotoCellByName means ...
> 
> Please! :) I meant "oSel.TextTable.getCellRangeByName(oSel.getRangeName())",
> "getCellRangeByName" being already mentioned here in the discussion, without
> any manual parsing.

OK, sorry, I missed that.