Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport release-3_40] [GUI] Replace Italic Font with Monospaced Font in Expression Fields of Attribute Table (#59139) #60143

Open
wants to merge 1 commit into
base: release-3_40
Choose a base branch
from

Conversation

lanckmann
Copy link
Contributor

This PR backports the changes from #59139 to the release-3_40 branch to resolve Issue #44638. The update replaces italic fonts with a monospaced font in the expression fields of the attribute table, enhancing readability and ensuring visual consistency across the interface.

… the attribute table. Related to issue qgis#44638  (qgis#59139)

* [gui] Remove italics and use monospaced font for expression fields in attribute tableThis commit modifies QgsFieldExpressionWidget to ensure that expression fields in the attribute table are displayed using a regular monospaced font instead of the default italics. This improves readability and ensures consistency between different parts of the interface.- Updated QgsFieldExpressionWidget::updateLineEditStyle() to set font to non-italic and apply monospaced family.- Addressed issue reported in qgis#44638 regarding the readability challenges caused by italicized font in expression fields.Related issue: qgis#44638

* [gui] Reformat code to match standard style
@github-actions github-actions bot added this to the 3.40.3 milestone Jan 14, 2025
Copy link

github-actions bot commented Jan 14, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit a0f3f56)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit a0f3f56)

@lanckmann
Copy link
Contributor Author

hey @nirvn , could you please take a look at my PR for the QGIS 3.40 backport? Let me know if there’s anything else you need. Thank you!

@agiudiceandrea agiudiceandrea requested a review from m-kuhn January 22, 2025 11:14
@@ -392,7 +392,8 @@ void QgsFieldExpressionWidget::updateLineEditStyle( const QString &expression )
currentField( &isExpression, &isValid );
}
QFont font = mCombo->lineEdit()->font();
font.setItalic( isExpression );
font.setFamily( ( QgsCodeEditor::getMonospaceFont() ).family() );
font.setItalic( false );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for the monospace font, but do we want to set italic to false also for expressions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea in the original issue was to replicate what is currently seen in the Expression Dialog where italic isn't used. To me, there isn't any benefit using italic in an expression as it can only make it harder to read and easier to misplace the cursor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think visualizing if something is a filed or expression is good, but there may be better options.
In the backport here, I'd leave it as italics.
In master we could try bold for fields and regular for expressions?

@nyalldawson
Copy link
Collaborator

Pinging @nirvn, who also has issues with the monospace change in master...

Copy link

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

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 15, 2025
@m-kuhn
Copy link
Member

m-kuhn commented Feb 15, 2025

Reping @nirvn

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 15, 2025
@nirvn
Copy link
Contributor

nirvn commented Feb 15, 2025

Hey there, sorry for not having opined on the original PR. I'm not a huge fan of the change and would consider reverting it prior to the release of 3.42. For one, it's causing some vertical alignment issue in the monospace font combo box over here, and it just looks generally out of place alongside other widgets in quite a few places, e.g.:

image

Also, I'm very much like @m-kuhn against removing the italic style when the expression is invalid. It provided a good visual feedback that's now just gone for no appealing reason.

@jfbourdon
Copy link
Contributor

I was suggesting the switch for monospace for the sake of concistency with the expression dialog window, but my real grunge is with the italic style that makes it difficult to position the cursor. Reverting to the current default font would be fine if the italic style is omitted. As for a way to alert the user of an invalid expression, would a change of color from black to red be a good alternative?

Copy link

github-actions bot commented Mar 2, 2025

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

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants