Named colors for dynamically applied palettes by NilsNemitz · Pull Request #1625 · pyqtgraph/pyqtgraph

NilsNemitz · 2021-03-07T17:55:52Z

pyqtgraph currently does not handle changes to the overall palette of colors really well (#1407 ).
#1588 points out that there is a useful paletteChange() signal that would allow an application to react to a switch between light and dark mode. However, any color changes beyond the simple background/foreground options would need to be applied manually.

This PR attempts to simplify this.
The example user-side code (in examples/PaletteApplicationExample.py) demonstrates a full palette replacement by a single line command like pg.palette.get('monogreen').apply()

This can be applied at any time, and automatically updates the displayed plots, even if no new data is sent.

The implementation exploits that basically all color requests go through functions mkColor() / mkPen() / mkBrush(). These can now return NamedPen or NamedBrush objects, subclassed from QPen and QBrush. These are assigned functional names such as 'gr_fg' for the graphical foreground color, or 'p0' to 'pB' for a set of twelve predefined plot colors. Such color names can be used directly in the pen= or brush= keyword arguments of the plotting functions.

When a new palette is assigned, NamedPen / NamedBrush objects automatically update the super-class QPen / QBrush object according to the new palette. As the named objects otherwise function in the same way, no large changes to the codebase are required. However, the subclasses of GraphicsObject, GraphicsWidget and GraphicsView now have a function styleHasChanged(), which is called on palette updates and can be used to perform additional tasks only when required. For example, ScatterPlotItem's SymbolAtlas needs to be cleared to rebuild in the new color set.

This PR demonstrates changes for most functionality in PlotCurveItem and ScatterPlotItem. If this method is adopted to improve color handling, there should be no major problems with including the remaining items. Some more cleanup is required.

I do not see any noticable impact on performance, since only minimal overhead is incurred from working with subclasses of QPen / QBrush. No additional code is executed except during the actual palette change.

The included palettes need some improvement, and would ultimately be user-definable and probably loaded from disk on request.

Comments and ideas are very welcome!

pyqtgraph/functions.py

j9ac9k · 2021-03-07T21:55:52Z

pyqtgraph/functions.py

-
-    pen = QtGui.QPen(QtGui.QBrush(color), width)
-    pen.setCosmetic(cosmetic)
+    assert qpen is not None        


probably don't want an assert in functional code; as they can be squashed/suppressed when you run python with the -O argument (which I would expect a fair chunk of our user-base to actually do).

Probably to something like:

if qpen is None: raise RuntimeError("some message")

qpen being None is really not supposed to ever happen here; it means I missed a possible execution path. This is an accidentally surviving debug statement.
But I guess throwing a recognizable error would be more helpful. Replaced by
if qpen is None:
raise ValueError('Failed to construct QPen from arguments '+str(args)+','+str(kargs) )

pyqtgraph/graphicsItems/GraphicsObject.py

pyqtgraph/namedColorManager.py

NilsNemitz · 2021-03-10T19:25:34Z

Dear @j9ac9k , thank you for the comments. I have updated the code accordingly.

The NamedPen and NamedBrush objects now take a manual reference to the NamedColorManager on init, that should remove the main cyclic imports.
I have added a compatibility function to intercept foreground / background setting through the legacy setConfigOption() method, and apply them directly to the palette.

… to placate code scanner

j9ac9k · 2021-04-04T03:48:55Z

examples/PaletteApplicationExample.py

+
+## Start Qt event loop
+if __name__ == '__main__':
+    QtWidgets.QApplication.instance().exec_()


I think we updated these since you created this PR, but put this line here instead: pg.mkQApp().exec_() (this handles the case there is no QApplication instance, which I know you declare earlier, but makes it easier)

pyqtgraph/__init__.py

j9ac9k · 2021-04-04T03:51:08Z

pyqtgraph/colormap.py

    return cm
-
+
+def make_monochrome(color='green'):


convert to camelCase please

j9ac9k · 2021-04-04T03:55:24Z

pyqtgraph/functions.py

+        args = args[0]
+    # now args is either a non-list entity, or a multi-element tuple
+    # short-circuits:
+    if isinstance(args, NamedBrush):


no code change requested here, just thinking of how nice the pattern matching functionality in python 3.10 will be able to serve us, ...too bad we won't get access to it for another ~4 years in this library.

j9ac9k · 2021-04-04T04:03:56Z

pyqtgraph/namedBrush.py

+        return not self.__eq__(other)
+
+    def __hash__(self):
+        return id(self)


We got burned really bad by this and had to fix that here: #1564

At the end of the day, QBrushs/QPens are mutable, and implementing __hash__ in such a way is our attempt to make them not mutable, which I think we're going to run into issues in general if we try and enforce that criteria.

This is only implemented here to allow the NamedColorManager to keep a list of weak references to all defined NamedColor objects. When a NamedColor object is deleted, the reference should be purged from he list immediately. A new object created with the same id would not be part of the list (or set).
But as discussed on Slack, this logic might be more straightforward to implement as a WeakValueDictionary, with id as the key. That should avoid providing a weak, limited-use hash function here.

j9ac9k · 2021-04-04T04:09:53Z

pyqtgraph/namedBrush.py

+        if DEBUG: print('  NamedBrush '+name+' updated to QColor ('+str(qcol.name())+', alpha='+str(alpha)+')')
+        super().setColor( qcol )
+
+    def paletteChange(self, color_dict):


named very similarly to the Signal paletteChanged, could we overwrite setPalette ... but it's not really a palette object coming in, maybe updatePalette?

I am undecided on this. This takes the place of what was originally intended to be a signal alerting all NamedColor objects of a palette change. Unfortunately QColor does not handle signals. I like that the naming still seems to indicate this function.

j9ac9k · 2021-04-04T04:11:24Z

pyqtgraph/namedPen.py

+        return not self.__eq__(other)
+
+    def __hash__(self):
+        return id(self)


see earlier comment about using the id() for __hash__

j9ac9k · 2021-04-04T04:20:32Z

Hi @NilsNemitz Sorry it's taken me a while to follow up on this. We've been busy 😈

First, I want to reiterate, this looks amazing, and I believe every effort getting this into pyqtgraph is very worthwhile.

In terms of code suggestion changes, I put most of my comments in-line, but I also would suggest changing some of the names of the signals and slots to be more in line with the Qt API in general, which we try and follow:

In this case this would mean:

paletteHasChangedSignal -> paletteChanged
styleHasChanged -> I'm not sure on this one, usually "changed" in the Qt API refers to a signal, but in this case I would think something like updateStyle would be appropriate. Looking into the method, they already have

One of the things that is bugging me a bit is it seems like we (and this isn't a we as in you, a few of us have fallen into this trap) are trying to treat the QPen, QBrush and QColor objects as if they're immutable. This is definitely not the case; the relevant objects can have their properties (and even colors) change. These items are not hashable, and any attempt to make them so will likely cause is trouble. I wish I could give more guidance here.

You have looked into the color-space here more than most of us by this point @NilsNemitz; do you see a solution of updating the existing QPen/QBrush/QColor values with new values to reflect the new colors we want to display less feasible than creating new instances of those objects?

NilsNemitz · 2021-04-04T12:46:37Z

Dear @j9ac9k, thank you very much for the review.
I am still working on this, and I'll do my best to include your suggestions in the next update!

Is there some Qt convention on names for functions that handle signals? styleHasChanged() is named that because it gest connected to the paletteHasChangedSignal() (but is slightly more general in function). As this would be added to many or most of the existing graphics items, I'd like to have a name that communicates this function as clearly as possible. Ideas?

updateStyle might work, but this is very close to the names of functions that are already in use, I think.

NilsNemitz · 2021-04-04T13:04:22Z

"trying to treat the QPen, QBrush and QColor objects as if they're immutable. "

I was originally hoping to use that feature to implement adjustable colors. However, that boat has already sailed:
Overall, PyQtGraph treats colors as fixed, seen most strongly in functions.mkColor(), which returns a copy of any QColor it is passed directly.

This treatment is supported by the behavior of the Python Qt bindings (or at least by PyQt5, which I am using): In the Qt c code, a QColor object can be passed to create e.g. a QPen object. This QColor object is kept intact, and the same object is returned by the getter function. So in principle, changing the original QColor object should affect the derived QPen and all colors that reference this QColor or QPen. In PyQt, this connection does not exist, as the getter function returns a copy of the original object (which I suspect is created on initializing the QPen).

I don't think the user can have any expectation of assigning a QColor and then achieving a useful response to changing its properties. Adding a NamedColor would provide precisely this functionality in a way that could be supported throughout the library.

The NamedColor / NamedPen / NamedBrush code is the thinnest wrapper I could come up with to extend the functionality of QColor / QPen / QBrush to locate and update them after their initial creation. If someone has a good idea how to make this even thinner, I'd be happy to pursue that.

If we can agree to ask the user to treat colors as immutable, then only one NamedColor object needs to exist for each (named) color and opacity combination, and locating the pre-existing objects for re-use would be done transparently by functions.mkColor().

We could consider disabling the related functions of NamedColor, but in the spirit of "we are all adults here", I am not sure if that is necessary, and it would add a lot of boilerplate code to block all the many methods that QColor provides. If we do this, then it might also be possible to simply disconnect such a modified NamedColor from future updates, which would revert it to the original color.

j9ac9k · 2021-04-04T20:58:34Z

Dear @j9ac9k, thank you very much for the review.
I am still working on this, and I'll do my best to include your suggestions in the next update!

Is there some Qt convention on names for functions that handle signals? styleHasChanged() is named that because it gest connected to the paletteHasChangedSignal() (but is slightly more general in function). As this would be added to many or most of the existing graphics items, I'd like to have a name that communicates this function as clearly as possible. Ideas?

updateStyle might work, but this is very close to the names of functions that are already in use, I think.

Yeah, this one is just via approximating; what I generally do is I look at the most equivalent/nearest Qt object, look at all the methods/signals and try to use a name that is in line with those. Here is a list of all the methods for QGraphicsItem: https://doc.qt.io/qt-5/qgraphicsitem-members.html

Signals are a bit tougher to track down, but I just know that somethingChanged is a pretty standard naming convention for Signals in Qt in general. PyQtGraph often follows the convention sigSomethingChanged as well. Collectively we try and follow Qt conventions whenever possible, but as there is no spec, it requires speculation.

I was originally hoping to use that feature to implement adjustable colors. However, that boat has already sailed:
Overall, PyQtGraph treats colors as fixed, seen most strongly in functions.mkColor(), which returns a copy of any QColor it is passed directly.

Blerg, I got myself crossed up with your implementation of __hash__, while the library uses colors as fixed, we have the ability to change that behavior in time. For the perspective of this PR, this may be a non-issue, as you do handle dynamic colors. Regarding the rest of the library, we have the capability of change that behavior given time/effort.

If we can agree to ask the user to treat colors as immutable,...

I think this is a pretty tall order; and is in conflict with the upstream framework, and may be in conflict with an application that pyqtgraph is being integrated into.

Also, there is some president for making wrappers for Qt objects like this in PyQtGraph; Point comes to mind: https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/Point.py Our normal convention for this wrappers is just to drop the Q; I'm not suggesting you go through the effort of changing NamedColor to Color yet, but after we settle on the API/philosophy, that is like a change that should be made before merging (unless we can think of a reason to keep the names as is).

j9ac9k · 2021-05-01T03:59:14Z

Hi @NilsNemitz

I gave this one a second look-over and I don't have any real concerns other than some naming mechanisms to get more in-line with the Qt API, which I can certainly help with. I am good with merging this once those are straightened out.

This is a big feature; and I suspect there are likely unintentional side-effects along the way; but I'm willing to sort those out as we go.

We may need to add some test code for this (test changing the global config option, check the colors of existing plots, that sort of thing).

j9ac9k · 2021-05-12T03:34:33Z

pyqtgraph/functions.py

+
+    err = 'Could not create a color from {:s}(type: {:s})'.format(str(args), str(type(args)))
+    result = COLOR_REGISTRY.getRegisteredColor(args)
+    if result is not None: # make a NamedPen


what's a named pen? ;)

No idea! --> changed

j9ac9k · 2021-05-12T03:40:51Z

pyqtgraph/palette.py

+        name = identifier
+        if color_dict is None or name not in color_dict:
+            if name[0] != '#':
+                raise ValueError('Undefined color name '+str(identifier))


would again recommend a f-string here f"Undefined color name {identifier}"

I will grudgingly admit that these seem quite useful. --> changed

j9ac9k · 2021-05-12T03:44:27Z

pyqtgraph/palette.py

+        #     's': QtGui.QColor(100,100,150,255)
+        # }
+        # self.addDefaultRamp()
+        self.dark = None # is initially set when assigning system palette


instead of setting to None, we can set default to what the current setup is:

https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/Qt.py#L417-L420

app = QtWidgets.QApplication.instance() self.dark = app.property('darkMode')

This way you shouldn't have to worry about the case of None

For the moment, each palette has a dark property, which currently simply represents that the background color is darker than (127,127,127). Since we are looking only at the plot palette here, this is not strongly linked to the system palette. PyQtGraph has been plotting dark plots in a light window for a long time.

As the comment says, this gets initialized immediately, so there is nothing to worry about. But not setting it makes the linter complain...

So I think this is okay here. We might want some method somewhere that sets the initial plot palette based on the system mode... But that would happen somewhere else. And for continuity, I think PyQtGraph should always start up with a dark plotting palette until instructed otherwise.

j9ac9k · 2021-05-12T03:45:50Z

pyqtgraph/palette.py

+                self.dark = bool( sum( bg_tuple[:3] ) < 3 * 127 ) # dark mode?
+            self.palette[key] = col
+
+    def addDefaultRamp(self):


should there be something in this function, or should it be removed?

For now, all custom gray colors are defined manually, but this function should replace that eventually.
Sorry, I forgot that this was left as a stub for now.

j9ac9k · 2021-05-12T03:49:13Z

pyqtgraph/palette.py

+        avail = ('m3','m4','m5','m6','m7','m8')
+        colors = {}
+        for idx, (value, key) in enumerate(brightness):
+            colors[key] = avail[idx]


dict comprehension time!

colors = {key: avail[idx] for idx, (_, key) in enumerate(brightness}

Actually, relooking at this, probably better off... (was guessing on variable names, please make better choices than I did 😆 )

colors = {key: avai for _, key, avai in zip(brightness, avail)

I think I'll change this to

choice = ('m3','m4','m5','m6','m7','m8') # 6 candidates for 6 needed colors colors = { key: choice[idx] # assign in order of dark to bright for idx, (_, key) in enumerate(brightness) }

It's not quite as compact, but I personally find it easier to see the ordering when the index is explicitly present.

j9ac9k · 2021-05-12T04:00:03Z

Hi @NilsNemitz I did another look through on the code; most of my changes now involve using list comprehensions instead, f-strings, things of that nature. I didn't make comments, but I think at this point it's ok to start deleting commented out code.

One thing I did want to discuss was naming of signals/slots.

Generally "somethingChanged" refers to a signal in Qt land; and you have "styleHasChanged" as a slot. I would recommend having stylePaletteChanged, an dI would suggest naming the slot setStyle but QGraphicsWidget.setStyle is already taken; we could do viewStylePaletteChanged and setViewStyle; or graphicsPaletteChanged and setGraphicsPalette perhaps. I'm open to other suggestions. Seems the bulk of the naming conflicts come from QGraphicsWidget.

NilsNemitz · 2021-05-16T15:36:20Z

Dear @j9ac9k , thanks for looking over this (too long) PR!

I tried to make some improvements based on your comments.
There are still items on the to-do list. The monochrome ramp generator is one, another would be to integrate the color map used to find plot colors with the intColor() call in pg.functions.

But this is more quality of life stuff and shouldn't change API or functionality much.

I renamed the signal graphStyleChanged and the slot updateGraphStyle, does that work better?
I think "graph" is not taken yet, and it seems an appropriate naming for PyQtGraph.

outofculture · 2021-07-17T01:37:43Z

pyqtgraph/Qt.py

    loadUiType = uic.loadUiType

    QtCore.Signal = QtCore.pyqtSignal
+    # QtCore.Slot   = QtCore.pyqtSlot # The current policy is to avoid slot decorators.


This line can be deleted, yeah?

outofculture · 2021-07-17T01:54:51Z

pyqtgraph/Qt.py

    loadUiType = uic.loadUiType

    QtCore.Signal = QtCore.pyqtSignal
+    # QtCore.Slot   = QtCore.pyqtSlot # The current policy is to avoid slot decorators.


This line can be deleted.

outofculture · 2021-07-17T01:55:25Z

pyqtgraph/__init__.py

+#     Each keyword argument sets one global option. 
+#     """
+#     for k,v in opts.items():
+#         setConfigOption(k, v)


All this commented code can just be deleted.

outofculture · 2021-07-17T02:19:26Z

pyqtgraph/styleRegistry.py

+
+del key, col # let the linter know that we are done with these
+
+def _expand_rgba_hex_string(hex):


This behavior (or very similar) is already implemented inside pyqtgraph.functions.mkColor, so we should try to refactor that out and share it.

Oscar438 · 2024-02-28T07:46:08Z

~~Not sure if this has been abondoned or not, but I'd love to see this!~~

Sorry, I believe this is the active version of this feature:
#2664

NilsNemitz added 5 commits March 4, 2021 15:34

skip unneded style updates in PlotDataItem

f97bff0

removed unwanted file

352039d

obliviated stray debug statement

547f847

start work on NamedBrush

eb6f724

mostly functional example

148a35d

j9ac9k reviewed Mar 7, 2021

View reviewed changes

pyqtgraph/functions.py Outdated Show resolved Hide resolved

j9ac9k reviewed Mar 7, 2021

View reviewed changes

pyqtgraph/graphicsItems/GraphicsObject.py Outdated Show resolved Hide resolved

j9ac9k reviewed Mar 7, 2021

View reviewed changes

pyqtgraph/namedColorManager.py Outdated Show resolved Hide resolved

NilsNemitz added 4 commits March 9, 2021 17:11

update

abda01c

restructured to reduce circular dependencies

291c7c3

Merge remote-tracking branch 'upstream/master' into named_colors

3414061

general cleanup

7d19711

NilsNemitz added 2 commits March 11, 2021 04:30

removed left over diagnostics, added redundant pre-definition of qcol…

f14e687

… to placate code scanner

added procedure generation for monochrome palettes

172cef1

NilsNemitz marked this pull request as draft March 14, 2021 11:52

added colormap sampling functions

244182d

j9ac9k reviewed Apr 4, 2021

View reviewed changes

pyqtgraph/__init__.py Show resolved Hide resolved

j9ac9k reviewed Apr 4, 2021

View reviewed changes

pyqtgraph/colormap.py Outdated

return cm

def make_monochrome(color='green'):

Copy link
Copy Markdown

Member

j9ac9k Apr 4, 2021

Choose a reason for hiding this comment

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

convert to camelCase please

j9ac9k reviewed Apr 4, 2021

View reviewed changes

j9ac9k reviewed May 12, 2021

View reviewed changes

NilsNemitz added 2 commits May 17, 2021 00:37

implemented some suggested improvements

1f8c82f

changes to monochrome code

87082ca

NilsNemitz mentioned this pull request May 23, 2021

Color map linearization #1797

Merged

j9ac9k mentioned this pull request Jun 9, 2021

[Feature request/proposal] Nice publication-ready default colors #1826

Open

NilsNemitz added 8 commits June 13, 2021 02:21

integrated with color ramp function in master, resolved conflicts

002e77a

retry solving merge conflict in Qt.py

df4a3c1

fixed enum styles

efc157f

add objects to registry BEFORE activating finalizer

9cd3067

spam some log messages

25bd251

updated more enums

7506397

MORE enums

1c49aef

do not timebomb other tests with orphaned finalize calls

0e06630

j9ac9k added the sprint Tag relating to issue/PR that is a good candidate for addressing during the sprints label Jul 8, 2021

outofculture reviewed Jul 17, 2021

View reviewed changes

This was referenced Jul 18, 2021

Extend ColorMap with HSL cycles and subset generation #1911

Merged

Factor out palette-related code from "named colors" PR #1921

Draft

This was referenced Mar 28, 2023

Add style sheet in pyqtgraph #2664

Closed

How do I style pyqtgraph to make it look like matplotlib？ #2673

Closed

NilsNemitz mentioned this pull request Sep 24, 2023

Preserve tick pen #2828

Merged

NilsNemitz mentioned this pull request Jun 30, 2024

Change background color in context menu #3080

Open


		del key, col # let the linter know that we are done with these

		def _expand_rgba_hex_string(hex):

Uh oh!

Conversation

NilsNemitz commented Mar 7, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NilsNemitz commented Mar 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j9ac9k commented Apr 4, 2021

Uh oh!

NilsNemitz commented Apr 4, 2021

Uh oh!

NilsNemitz commented Apr 4, 2021

Uh oh!

j9ac9k commented Apr 4, 2021

Uh oh!

j9ac9k commented May 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j9ac9k commented May 12, 2021

Uh oh!

NilsNemitz commented May 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oscar438 commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Oscar438 commented Feb 28, 2024 •

edited

Loading