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

[Richard Gustavsen]

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.

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. 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).

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:


let cell = 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.


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.

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. 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)

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.

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.


-Richard


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20221219/000d462a/attachment.htm>