On 19/12/2022 11:18, Richard Gustavsen wrote:

Item itemAtCell(int column, int row)

positionViewAtCell(int column, int row, PositionMode mode, point offset, rect 
subRect)

To me this is quite weird. This must be the *only* place in Qt model/view APIs 
where (column,row) is used instead of (row,column), and I don't seem to find 
any justification for this.

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.

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?



Also the above functions are also inconsistently (and weirdly) overloaded with 
overloads taking `point` (== QPointF). For instance:


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:

https://github.com/qt/qtdeclarative/blob/dev/src/quick/items/qquicktableview.cpp#L5950

In other words, you're not supposed to pass, say, the coordinates of a mouse click (a _`point`_ indeed!) to it in order to get the model index of the clicked cell.

Since it's a logical index, it makes absolutely no sense to accept a floating point coordinate there. Why even offering this "convenience"?

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 alone, ever -- I've always carried around a model index when I needed to identify a cell.)




positionViewAtCell(point cell, PositionMode mode, point offset, rect subRect)

Here `cell` is a logical index (so why do they take a FP-based point?), and for 
extra fun, cell.x is the *column* and cell.y is the *row* (!!!).

Why do you find it “fun" that x maps to column and y maps to rows in this case? 
To me that is logical. Do you think x should map to rows instead?

And finally:

point cellAtPosition(point position, bool includeSpacing)

This is pure evil, as `position` is a position in pixels (relative to the 
TableView's content item), while the return is ... a logical index (with again 
`x` being the column and `y` and the row). The type of the parameter and the 
return value is the same, yet they represent completely different things! How 
is this good API?

The API uses the term “position” whenever we talk about a pixel position, and 
“cell” whenever we talk about a table cell.This is consistent through out the 
API. The fact that we use point as type for them both should not be confusing 
when the function is called cellAtPosition. What else could _cell_ mean? Pixel 
position?

Because (as far as TableView is concerned) sometimes `point` means a coordinate in (fp-based) pixels, and sometimes a logical (int-based) position in a model.

I have a function that returns a `point`. Which TableView methods are OK to call with it? I can't tell by the type alone; that's a red flag in API design, since one could easily fix this by having distinct types.



So, how to fix the above? One could easily fix the API break, but one is left 
with broken APIs all over the place.

You then assume that the API is semantically broken all over the place (not 
SIC), and it has been like for in the whole of Qt 6. I don’t think so. But if 
others agree, I would say that this is something to look at in an upcoming 
version of Qt. Not Qt 6.5?

If TableView was written from the start, the API for itemAtCell would have been 
row, column and not column, row. Not because of the arguments above, but simply 
because it would be more consistent with the order you specify the coordinates 
to a QModelIndex. Whether you specify row first, or column first, in the API is 
not (in my eyes) wrong or right, but a matter of preference. But it should be 
consistent.

Between 6.3 and 6.4 there's been an API break in QtQuick's TreeView: its 
modelIndex(row,column) function got moved into its base class (TableView), and 
the arguments swapped for it, so now it's TableView::modelIndex(column,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.

Thanks,
--
Giuseppe D'Angelo | giuseppe.dang...@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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

_______________________________________________
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development

Reply via email to