Add "Show Full Text" option to Identify Settings #62946
Add "Show Full Text" option to Identify Settings #62946lanckmann wants to merge 24 commits intoqgis:masterfrom
Conversation
🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. 🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. 🪟 Windows buildsDownload Windows builds of this PR for testing. |
src/app/qgsidentifyresultsdialog.cpp
Outdated
| if ( showFullText ) | ||
| { | ||
| // Create a multi-line label widget to show full text | ||
| QLabel *valueLabel = new QLabel( representedValue ); | ||
| valueLabel->setWordWrap( true ); | ||
| valueLabel->setAlignment( Qt::AlignTop | Qt::AlignLeft ); | ||
| valueLabel->setTextInteractionFlags( Qt::TextSelectableByMouse ); | ||
| valueLabel->setStyleSheet( QStringLiteral( "QLabel { background: transparent; }" ) ); | ||
|
|
||
| attrItem->setData( 1, Qt::DisplayRole, QString() ); | ||
| QTreeWidget *tw = attrItem->treeWidget(); | ||
| tw->setItemWidget( attrItem, 1, valueLabel ); | ||
| } | ||
| else | ||
| { | ||
| attrItem->setData( 1, Qt::DisplayRole, representedValue ); | ||
| QTreeWidget *tw = attrItem->treeWidget(); | ||
| tw->setItemWidget( attrItem, 1, nullptr ); | ||
| } |
There was a problem hiding this comment.
| if ( showFullText ) | |
| { | |
| // Create a multi-line label widget to show full text | |
| QLabel *valueLabel = new QLabel( representedValue ); | |
| valueLabel->setWordWrap( true ); | |
| valueLabel->setAlignment( Qt::AlignTop | Qt::AlignLeft ); | |
| valueLabel->setTextInteractionFlags( Qt::TextSelectableByMouse ); | |
| valueLabel->setStyleSheet( QStringLiteral( "QLabel { background: transparent; }" ) ); | |
| attrItem->setData( 1, Qt::DisplayRole, QString() ); | |
| QTreeWidget *tw = attrItem->treeWidget(); | |
| tw->setItemWidget( attrItem, 1, valueLabel ); | |
| } | |
| else | |
| { | |
| attrItem->setData( 1, Qt::DisplayRole, representedValue ); | |
| QTreeWidget *tw = attrItem->treeWidget(); | |
| tw->setItemWidget( attrItem, 1, nullptr ); | |
| } | |
| // Create a multi-line label widget to show full text | |
| QLabel *valueLabel = new QLabel( representedValue ); | |
| valueLabel->setAlignment( Qt::AlignTop | Qt::AlignLeft ); | |
| valueLabel->setTextInteractionFlags( Qt::TextSelectableByMouse ); | |
| valueLabel->setStyleSheet( QStringLiteral( "QLabel { background: transparent; }" ) ); | |
| if ( showFullText ) | |
| { | |
| valueLabel->setWordWrap( true ); | |
| } | |
| attrItem->setData( 1, Qt::DisplayRole, QString() ); | |
| QTreeWidget *tw = attrItem->treeWidget(); | |
| tw->setItemWidget( attrItem, 1, valueLabel ); |
Would it work like this as well? Or would it not set the ellipsis in the right way?
src/app/qgsidentifyresultsdialog.cpp
Outdated
| QLabel *valueLabel = new QLabel( fullText ); | ||
| valueLabel->setWordWrap( true ); | ||
| valueLabel->setAlignment( Qt::AlignTop | Qt::AlignLeft ); | ||
| valueLabel->setTextInteractionFlags( Qt::TextSelectableByMouse ); | ||
| valueLabel->setStyleSheet( QStringLiteral( "QLabel { background: transparent; }" ) ); | ||
|
|
||
|
|
||
| item->setData( 1, Qt::DisplayRole, QString() ); |
There was a problem hiding this comment.
A little bit duplicated code. If someone changes something in the styling (like background) it has to be done here and on QgsIdentifyResultsDialog. Maybe this can be improved. What do you think?
src/app/qgsidentifyresultsdialog.cpp
Outdated
| // Always create a label widget for consistent styling and text selection | ||
| QLabel *valueLabel = createStyledLabel( representedValue, showFullText ); | ||
| attrItem->setText( 1, representedValue ); | ||
| attrItem->setData( 1, Qt::DisplayRole, representedValue ); |
There was a problem hiding this comment.
in the foundLinks == true branch we set the DisplayRole to QString() -- I believe that should also be done here, otherwise the transparent background for the label will result in overlapping text
src/app/qgsidentifyresultsdialog.cpp
Outdated
| layItem->setText( 0, QStringLiteral( "%1 %2" ).arg( vlayer->name(), countSuffix ) ); | ||
|
|
||
|
|
||
| updateTextDisplay(); |
There was a problem hiding this comment.
This shouldn't be called here -- this method will be called many times if multiple features are identified, and it's iterating through everything in the list for every feature added. That'll add up quickly.
c92b6f9 to
adae752
Compare
14e969b to
d38177e
Compare
signedav
left a comment
There was a problem hiding this comment.
Hi @lanckmann
It's a nice option, but there are some parts I don't get why it's done like it's done (see comments).
The label works not like expected btw. on my system (I don't have the resources to evaluate the reason atm):
src/app/qgsidentifyresultsdialog.cpp
Outdated
| } | ||
| } | ||
|
|
||
| QLabel *QgsIdentifyResultsDialog::createStyledLabel( const QString &text, bool wordWrap ) |
There was a problem hiding this comment.
is there a case where createStyledLabel should not wordWrap?
There was a problem hiding this comment.
I thought I might need two different label styles like the first with word wrap for long text and the second without word wrap for short text or other use cases. So i added the "wordWrap" parameter thinking it would make the function more flexible.
src/app/qgsidentifyresultsdialog.cpp
Outdated
| : item->text( 1 ); | ||
|
|
||
| QTreeWidget *treeWidget = item->treeWidget(); | ||
| const int lengthThreshold = 100; // Characters |
There was a problem hiding this comment.
This is confusing. Why 100? 100 is usually truncated...
It should be the width of the panel. Means if it's truncated, then it makes sense to wrap, no?
Or why not wrap it always when this setting is set?
There was a problem hiding this comment.
I wanted to avoid creating QLabel widgets for short text like "abc" or "123", thinking it would be overhead. So I added a 100 char threshold (width calcul) .
src/app/qgsidentifyresultsdialog.cpp
Outdated
| const QFontMetrics fm( valueLabel->font() ); | ||
| const int avgCharWidth = fm.averageCharWidth(); | ||
| const int maxCharsPerLine = 100; // Reasonable column width estimate | ||
| const int maxLines = 20; |
There was a problem hiding this comment.
What happens, when it's more?
There was a problem hiding this comment.
I was worried about performance issues with very long text values. In some datasets, attribute fields can contain huge amounts of text
|
Ok, but I'm quite busy at the moment. I'll need some time for another review. |
- Ensure setText() is called before setItemWidget() to preserve item text - Fix TestQgsIdentify::closestPoint() expecting col1Item->text(1) to return value - Maintain backward compatibility with existing test expectations
- Ensure setText() is called before setItemWidget() to preserve item text - Fix TestQgsIdentify::closestPoint() expecting col1Item->text(1) to return value - Maintain backward compatibility with existing test expectations
…isplayRole to QString() to prevent text overlap with transparent label backgrounds- Move updateTextDisplay() call from addFeature() to updateViewModes() to avoid repeated iterations when multiple features are identified. reviewer feedback
93949b9 to
eb90a50
Compare
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
No stale. But not sure if I find time to review before Christmas. Sorry, about that. There are quite some pieces I need to reproduce to fully be able to confirm. |
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
Not stale |
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
No stale |
Add 'Show full text' option to Identify Settings
Adds a new configurable option in the Identify Results dialog settings menu that allows to toggle between truncated text display (with "...") and full text display on multiple lines.
Added settingShowFullText boolean setting entry.
added "Show Full Text" checkbox to Identify Settings menu.
Implemented text display switching using QLabel widgets with word wrap.
Fixes #56167