[layout] allow sorting attribute table by field not listed in the table #36236

3nids · 2020-05-06T12:52:53Z

instead of using the same data model for the displayed and the sorting columns, two data models are now used.
They use the same API / base class as they are very similar.
QgsLayoutTableColumn is no longer a QObject.

this fixes #25671

a new feature with less lines in the code base 💪

fixes qgis#25671 Instead of using a sort filter proxy model on top of the columns model, this uses a dedicated model for the sorting columns

for sake of simplicity this can live in the same class than the disaply columns

qgis-bot · 2020-05-06T13:38:52Z

@3nids
This pull request has been tagged as requiring documentation.

A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged.

Please update the description (not the comments) with helpful description and screenshot to help the work from documentors.
Also, any commit having [needs-doc] or [Needs Documentation] in will see its message pushed to the issue, so please be as verbose as you can.

Thank you!

nyalldawson

Looking good! Great feature, and love the code cleanups!

nyalldawson · 2020-05-06T18:10:11Z

src/core/layout/qgscompositionconverter.cpp

+
+      // sorting columns are now (QGIS 3.14+) handled in a dedicated list
+      Q_NOWARN_DEPRECATED_PUSH
+      for ( const QgsLayoutTableColumn &col : qgis::as_const( layoutItem->mColumns ) )


this would be easier to read/safer if you enclosed the loop contents in { / }

I will switch back to std::copy_if then

nyalldawson · 2020-05-06T18:11:53Z

src/core/layout/qgslayoutitemattributetable.cpp

@@ -368,16 +367,16 @@ void QgsLayoutItemAttributeTable::restoreFieldAliasMap( const QMap<int, QString>
    return;
  }

-  for ( QgsLayoutTableColumn *column : qgis::as_const( mColumns ) )
+  for ( QgsLayoutTableColumn &column : mColumns )


My understanding is that you're NOT allowed to modify a qt container inside a range based loop. Can you confirm that this is safe?

correct, I will fix this

nyalldawson · 2020-05-06T18:15:05Z

src/core/layout/qgslayoutitemattributetable.cpp

@@ -480,10 +479,9 @@ bool QgsLayoutItemAttributeTable::getTableContents( QgsLayoutTableContents &cont
    req.setFilterFid( atlasFeature.id() );
  }

-  QVector< QPair<int, bool> > sortColumns = sortAttributes();
-  for ( int i = sortColumns.size() - 1; i >= 0; --i )
+  for ( const QgsLayoutTableColumn &column : qgis::as_const( mSortColumns ) )


I think there's an api break here -- if someone is using the old api to set a list of columns with their sort order contained, then that won't be respected here.

Then setColumns would be used, right?
When using setColumns, I can check if a sorting order is provided and call setSortColumns at the same time.
Works for you?

I thought about that too- but wouldn't it be hard to differentiate between a call to setColumns using the old API, vs correct use of the new api and calling setSortColumns and then later setColumns?

Well, if you correctly use the API there is no sorting info (rank) in the list when calling setColumns. If there is, then you're using the old API and we call setSortColumns.

src/core/layout/qgslayouttable.cpp

nyalldawson · 2020-05-06T18:18:42Z

src/core/layout/qgslayouttable.h

+ * List of column definitions for sorting a QgsLayoutTable
+ * \since QGIS 3.14
+*/
+typedef QVector<QgsLayoutTableColumn> QgsLayoutTableSortColumns;


Reusing QgsLayoutTableColumn seems wasteful here -- that class stores a lot more than the sort related attributes. What do you think about splitting the sort related attributes from QgsLayoutTableColumn into a new dedicated class?

That's what I did on first run.
But it brings some nice advantage for the code

that we can have a single model class instead of two

less deprecation of methods (1), no need to deprecate the use of the class for another one in certain circumstances

no need to cast in backward project compatibility (and API if we handle this in setColumns)

On the other hand we're talking about 3 enums and 1 double values used for nothing for a couple of columns.

I'd vote to stick as is (my agenda votes for it too ;) )

Ok, proceed....!

src/gui/layout/qgslayoutattributeselectiondialog.cpp

… sharing

3nids · 2020-05-06T19:36:49Z

review comments have been addressed.

qgis-bot · 2020-05-07T04:03:02Z

@3nids
A documentation ticket has been opened at qgis/QGIS-Documentation#5423
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

nirvn · 2020-05-07T04:04:15Z

@3nids , nice one!

3nids added 7 commits May 6, 2020 14:46

allow sorting attribute table by field not listed in the table

fcea343

fixes qgis#25671 Instead of using a sort filter proxy model on top of the columns model, this uses a dedicated model for the sorting columns

do not use a specific class for the sort columns

03f4cb9

for sake of simplicity this can live in the same class than the disaply columns

fix backward compatibility and fix leak

525661c

fix crash - clone the columns

e7fe141

QgsLayoutTableColumn is not a QObject anymore

ee686b5

regroup the 2 models within an abstract base class

ab4e179

adapt test

2b252c8

3nids requested a review from nyalldawson May 6, 2020 12:52

remove duplicated forward declaration*

971d107

github-actions bot added this to the 3.14.0 milestone May 6, 2020

3nids added Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. labels May 6, 2020

fix dox and layout

28c20cf

3nids force-pushed the fix25671 branch from a5e1c01 to 28c20cf Compare May 6, 2020 18:01

nyalldawson reviewed May 6, 2020

View reviewed changes

API backward compatibility fix, code layout and readability, implicit…

51b0d5a

… sharing

3nids merged commit 7a172b5 into qgis:master May 7, 2020

3nids deleted the fix25671 branch May 7, 2020 04:02

qgis-bot mentioned this pull request May 7, 2020

[layout] allow sorting attribute table by field not listed in the table (Request in QGIS) qgis/QGIS-Documentation#5423

Closed

pigreco added the Print Layouts Related to QGIS Print Layouts, Atlas or Reporting frameworks label May 12, 2020

timlinux added Changelog Items that are queued to appear in the visual changelog - remove after harvesting and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels May 22, 2020

3nids mentioned this pull request Jun 18, 2020

Sorting on hidden fields in attribute table in print layout opengisch/QGIS-Usability-improvement-roundhouse-kick#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[layout] allow sorting attribute table by field not listed in the table #36236

[layout] allow sorting attribute table by field not listed in the table #36236

3nids commented May 6, 2020

qgis-bot commented May 6, 2020

nyalldawson left a comment

nyalldawson May 6, 2020

3nids May 6, 2020

nyalldawson May 6, 2020

3nids May 6, 2020

nyalldawson May 6, 2020

3nids May 6, 2020

nyalldawson May 6, 2020

3nids May 6, 2020

nyalldawson May 6, 2020

3nids May 6, 2020

nyalldawson May 6, 2020

3nids commented May 6, 2020

qgis-bot commented May 7, 2020

nirvn commented May 7, 2020

[layout] allow sorting attribute table by field not listed in the table #36236

[layout] allow sorting attribute table by field not listed in the table #36236

Conversation

3nids commented May 6, 2020

qgis-bot commented May 6, 2020

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3nids commented May 6, 2020

qgis-bot commented May 7, 2020

nirvn commented May 7, 2020