[Development] How to fix TableView's API break?

Giuseppe D'Angelo giuseppe.dangelo at kdab.com
Tue Dec 20 19:45:06 CET 2022


Hi,
Il 19/12/22 18:21, Richard Gustavsen ha scritto:
>
>>>> Does anyone know?
>>> The rationale behind it is that you normally would like to address a 
>>> cell in a table using x, y coordinates. x maps horizontally and y 
>>> maps vertically (to be consistent with e.g Item.x and Item.y). This 
>>> means that x maps to columns and y maps to rows. itemAtCell(column, 
>>> row) follows that thinking, since that is on the same format as 
>>> writing itemAtCell(x, y).
>>
>> Well, for starters, nowhere in Qt one has ever referred to the 
>> _logical positions_ in a model/view using "x" and "y", but "row" and 
>> "column". "x" and "y" are usually for things like pixel positions.
>
> Which is fine. You can always just the API that takes a column and a 
> row instead of a cell, if that makes most sense for a particular 
> application. The API has overloads for both column and row, and QPoint.

That's not the point I'm making; when it comes to positions in a model 
it's "row" and "column", not "column" and "row", vs. it's naturally "x" 
and "y" (and not y and x) when it comes to QPoints (yes, I know that 
story about Mac).


>
>> Row-major or column-major access is surely a case of tomayto/tomahto, 
>> but why deciding to introduce an inconsistency in TableView, after 
>> decades of APIs built around accessing via row/column?
>
> It’s what I wrote above. But TableView was introduced more than four 
> years ago, so there might have been other considerations as well.

That's why I asked if anyone knew the reasoning...


> But anyway, once this path was taken, any new functions that we add 
> (like modelIndex()) should follow the same template, to be consistent. 
> And to me, this API should be 100% consistent, in that respect. You 
> mention the word “consistent” several times, but if I understand you 
> correctly, is more about why row and column are specified in the 
> “wrong” order. (BTW, this affects three functions in TableView, out of 
> more than thirty, to put things in perspective). And also why the API 
> uses QPoint to represent a cell. Both are valid concerns (I’ll fill in 
> a bit more below), but it's not inconsistent, it has been like this 
> for a long time, and should not need urgent attention (AFAICS).

I use the word because there are two levels of inconsistencies at play:

1. (current) TableView API vs. the rest of Qt's model/view classes: the 
former uses (column,row), the latter (row,column);

2. if we swap modelIndex' arguments: modelIndex (row,column) vs. the 
rest of TableView API that still has (column,row).

Neither is ideal and I would've strongly preferred to stick to 
(row,column) everywhere. Everything else is an API trap waiting to happen.

(Other ideas: remove the row/column convenience, and always use indices. 
Why does a _view_ return model indices at a given logical position, and 
not the model directly? Etc.)


>
>>>>> QModelIndex modelIndex(point cell)
>>> A point is specified using Qt.point(x, y), so again consistent with 
>>> what I wrote above. I don’t find it weird that the API lets you also 
>>> combine x and y coordinates into a point type.
>>
>> But this is precisely why it's weird and confusing. `point` here 
>> isn't a point (as in, floating point coordinates on a Cartesian 
>> plane; aka QPointF). It's the logical index of the cell:
>>
>> Since it's a logical index, it makes absolutely no sense to accept a 
>> floating point coordinate there. Why even offering this "convenience"?
>
> Consider this:
>
> letcell=tableView.cellAtPosition(tapEvent.position)
> tableView.positionViewAtCell(cell,TableView.AlignCenter)
> This short example shows why working with a cell/coordinate, rather 
> that individual (column, row), can be beneficial. It makes things more 
> readable, and requires fewer lines of code. The first line shows also 
> that having a type that can represent a cell is useful, since 
> cellAtPosition() can only return a single value.

This is spelled "model index" in the entirety of Qt model/view code. Why 
can't the above be implemented like this?

let index = tableView.indexAtPosition(tapEvent.position) // this is a 
QModelIndex
tableView.positionViewAtIndex(index)


Modulo API renames, this 100% matches existing practices 
(QAbstractItemView::indexAt, scrollTo, etc.). The win here is that 
`index` is not a point, and it has unambiguous meaning: a logical 
position in a model. You can still extract row/column information from 
QML if you want to. You can pass it to C++ and get a QModelIndex and use 
it to manipulate your model. Also, you can't accidentally pass `index` 
to the wrong thing:


tableView.positionViewAtIndex(tapEvent.position); // ERROR: wrong 
argument type
tableView.positionViewAtCell(tapEvent.position);  // OK, but BUG! Wanted 
a logical index, gets a pixel position


> Note that in the qquicktableview_p.h API, a QPoint is used to 
> represent a cells, and not QPointF. The fact that this gets translated 
> to a floating point in QML is off-topic, or at least not much that I 
> can do about in TableView.

Can't TableView expose a dedicated Q_GADGET type with row/column if it 
needs one? (But why, when there's QModelIndex anyways?)

Wait, is this the reason -- that QModelIndex got "gadgetified" 
relatively recently (5.6?), and these views have been around since 
before it and couldn't use it?


>
>> And why accepting, specifically, `point`? I can't help but think that 
>> this is another case of shoehorning -- QPoint is "the first available 
>> datatype that gets the job done". This use case demanded a brand new 
>> value type and that's tedious to do. (But did it demand one? I've 
>> never had the need to use a (row,column) pair
>
> You could argue that we need a new type for this, and perhaps you’re 
> right. But a table is, after all, just a two-dimensional grid of 
> cells, and QPoint is meant as a general type for dealing with all 
> kinds of two-dimensional coordinate systems.

That's very misleading.

For a cell position there's no sense of asking stuff like "what's the 
Manhattan distance from the origin" or "multiply its coordinates by a 
scalar factor", which are instead legitimate operations on a type like 
QPoint. A cell position simply is not a point, although it has the same 
physical structure (two ints). But 1) logically those two ints are used 
in a very different way (x,y vs row,column); and 2) set of operations of 
the two classes isn't the same. And that's fine, precisely because they 
don't model the same entity.

It's the same story of "don't use pair<int,int>, use a named struct" in C++.


> And QML has language support for doing arithmetics with points. So I’m 
> not convinced that introducing a new type is correct. I’m not stubborn 
> about this, and I’m open for ideas/discussion (but perhaps in JIRA)

In fact I wouldn't propose a "new" type, I would just propose to stick 
with QModelIndex.


>
>> alone, ever -- I've always carried around a model index when I needed 
>> to identify a cell.)
>
> This has todo with legacy reasons, all the way to ListView. Since you 
> can assign many different kinds of models to a ListView and TableView; 
> js arrays, ObjectModel, string lists, numbers, etc, so there isn’t 
> always a QAbstractItemModel in use. Hence, you cannot always rely on a 
> cell being backed by a QModelIndex.


> If the whole QML Item Views story were to be rewritten, I would argue 
> that _all_ supported models should have beed proxied by a QAIM. But 
> that's not the current situation.

This sounds like something very desirable (why isn't it done that way?); 
on the other hand, I wonder how much non-prototyping code is not using a 
QAIM.

But yes, ListView (and Repeater, ...) have always been a jack of all 
trades and master of none in this respect. The int-based indexing that 
one gets in a delegate is convenient, until it is not, and one has 
always to reach back to the model to turn it into a QModelIndex to do 
something useful with it (e.g. on the C++ side). It should've always 
been a QModelIndex. Integrating something like a selection model into a 
ListView is a nightmare (gets done at the model level, sometimes through 
a identity proxy model, than at the view level).


>
> It’s also worth mentioning that a TableView is not only meant for 
> typical spread sheet applications. We know from experience with 
> ListView, that it might end up being (mis)used for all kinds of 
> things. e.g ListView is also to implement a PageView etc. And we also 
> have a couple of examples in Qt today, gameOfLife and pixelator, that 
> shows a more “mathematical” usage of TableView, where perhaps x and y 
> makes more sense that column and row.
>
>>> That is a problem, yes. Good catch. Not sure how that sneaked in, 
>>> but it needs to be fixed. But this doesn’t require an exception to 
>>> the FF, we can surely fix up the API (added to Qt 6.5) after the FF?
>>
>> Well, note that this is a regression in 6.4, so it needs to be fixed 
>> there. But re-swapping the arguments will break 6.4.0/6.4.N source 
>> compatibility for TableView. And introduce an API flaw due to being 
>> inconsistent with the other methods; that's why I was wondering if 
>> one should just tackle them all in one go.
>
> TBH, I’m not sure how to fix that one. QQuickTreeView was introduced 
> in Qt 6.3, and the offending function was moved to QQuickTableView in 
> Qt 6.4. The arguments got swapped in the process, exactly to be 
> consistent with the rest of the TableView API (that is, the two other 
> functions that takes a column and a row as argument. Changing that 
> back in Qt 6.5 wouldn’t help much, since we would still end up with a 
> Qt version that is different from the rest (Qt 6.4) And then we also 
> get a function that is inconsistent with two other function. Any idea 
> is welcome.
>
Well, thus this email thread, I also didn't have any idea there. But 
thank you for the submitted patch!


-- 
Giuseppe D'Angelo |giuseppe.dangelo at kdab.com  | Senior Software Engineer
KDAB (France) S.A.S., a KDAB Group company
Tel. France +33 (0)4 90 84 08 53,http://www.kdab.com
KDAB - The Qt, C++ and OpenGL Experts
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20221220/b6d9ca5e/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4244 bytes
Desc: Firma crittografica S/MIME
URL: <http://lists.qt-project.org/pipermail/development/attachments/20221220/b6d9ca5e/attachment-0001.bin>


More information about the Development mailing list