Hi, Il 19/12/22 18:21, Richard Gustavsen ha scritto:
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).Does anyone know?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.)
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.QModelIndex modelIndex(point cell)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) pairYou 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).
Well, thus this email thread, I also didn't have any idea there. But thank you for the submitted patch!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.
-- 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: Firma crittografica S/MIME
_______________________________________________ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
