Skip to content

Use QColor.fromString instead of deprecated QColor.setNamedColor#2877

Merged
j9ac9k merged 6 commits intopyqtgraph:masterfrom
zariiii9003:QColor_fromString
Feb 14, 2024
Merged

Use QColor.fromString instead of deprecated QColor.setNamedColor#2877
j9ac9k merged 6 commits intopyqtgraph:masterfrom
zariiii9003:QColor_fromString

Conversation

@zariiii9003
Copy link
Copy Markdown
Contributor

QColor.setNamedColor is deprecated since Qt 6.6. Use QColor.fromString if available.

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Nov 11, 2023

We could use the constructor form instead, which is not deprecated and works across all supported versions, and thus avoid having to differentiate between versions.

>>> QtGui.QColor("steelblue")

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 11, 2023

Thanks for the PR @zariiii9003 ...appreciate the lookout for deprecated methods!

Probably should go w/ the constructor form instead, QColor::fromString was only introduced in Qt 6.4, so instead of adding if-checks for hasattr using the constructor, using the QColor(namedColor) instead would probably e simpler

@zariiii9003
Copy link
Copy Markdown
Contributor Author

Thanks for your comments, i'll update the PR soon

@zariiii9003 zariiii9003 force-pushed the QColor_fromString branch 2 times, most recently from 7e38ec8 to 95b1597 Compare November 13, 2023 19:34
@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Nov 14, 2023

Your changes have made the following to no longer work:

In [1]: import pyqtgraph as pg
In [2]: pg.mkColor('steelblue')
Out[2]: PySide6.QtGui.QColor.fromRgbF(0.274510, 0.509804, 0.705882, 1.000000)

Referring to:
#1944 (comment)
it seems that this feature was left undocumented and "experimental"

qcol = QtGui.QColor()
qcol.setNamedColor(c)
# match hex color codes
match = re.match(r"#([0-9a-fA-F]{3,8})", c)
Copy link
Copy Markdown
Contributor

@NilsNemitz NilsNemitz Nov 14, 2023

Choose a reason for hiding this comment

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

My personal impression is that it might be better to try avoiding the overhead of running the regex parser just to confirm the hex nature of the input string. As a relatively low level function, I am sure that there's a use case where mkColor gets called a lot.
Can we fix the deprecation problem with setNamedColor by using qcol = QtGui.QColor(c)?

if qcol.isValid():
return qcol
# on failure, fallback to pyqtgraph parsing
# this includes the deprecated case of non-#-prefixed hex strings
Copy link
Copy Markdown
Contributor

@NilsNemitz NilsNemitz Nov 14, 2023

Choose a reason for hiding this comment

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

Did we intend to throw a deprecation warning when the code falls through into the path for non #-prefixed hex codes?

Hex codes without leading '#' character have been out of the documentation for a while (two years?) now, but it seems that we've continued to just silently accept them.

If we change the code to handle all valid string cases ('#'-prefixed hex and valid color names) above, then we can raise a deprecation warning here, and it should only trigger for hex strings with missing prefix.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did we intend to throw a deprecation warning when the code falls through into the path for non #-prefixed hex codes?

We killed support for this in 0.13.0 but I think it still works...

https://github.com/pyqtgraph/pyqtgraph/blob/pyqtgraph-0.12.4/pyqtgraph/functions.py#L272-L276

have_alpha = len(c) in [5, 9] and c[0] == '#' # "#RGBA" and "#RRGGBBAA"
if not have_alpha:
# try parsing SVG named colors, including "#RGB" and "#RRGGBB".
# note that QColor.setNamedColor() treats a 9-char hex string as "#AARRGGBB".
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could convert pyqtgraph's internal #RRGGBBAA hex string format to Qt's #AARRGGBB format by simply reshuffling the string, then the code could go through the QColor(string) constructor in all cases where the input is a string.

If length is '#' + 4, then the format is RGBA and needs to be expanded manually by e.g. the neat "".join([x + x for x in c]) construction used below.

After that, or when length is '#' + 8, then reshuffle c = '#' + c[8:10] + c[1:7]

Then, QColor(c) should handle everything.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Feb 14, 2024

Hi @zariiii9003

Thanks for the PR, this LGTM, I'm about to merge. I wanted to apologize for the delay on the merge.

Thanks!

@j9ac9k j9ac9k merged commit 8b37a1c into pyqtgraph:master Feb 14, 2024
@zariiii9003 zariiii9003 deleted the QColor_fromString branch February 15, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants