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
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
