change GroupParameterItem palette to address issue in darkmode on mac#2101
change GroupParameterItem palette to address issue in darkmode on mac#2101j9ac9k merged 11 commits intopyqtgraph:masterfrom Wubbzi:paramtreeStyle
Conversation
|
A fellow macOS user has entered the chat! Thanks for tracking this down @Wubbzi Over the next few days I'll try and test out this change on the other platforms. |
|
|
|
One thing, which is bothering me is why on Your Mac at light theme the font at 0 level gets dark dark, and on my linux KDE neon and conda it is not getting dark at all. It is still readable, but the font color does not kick in, Your code suppose to change that but it does not. I tested if app.property("darkMode") is working, it is. As far as I recall from my other issues (with my software) somehow the font color in my cases could not be directly changed. Changing Font color worked through stylesheets only. Probably it is Qt for linux thing... I am surprised that set color is ignored, but Qt still manages to set color which is enough contrasting. P.S. thanks to this Issue I got aware that there is this property "darkMode", in my own application I had unnecessary made own function to check for that (checking if fg color is lighter than bg). Good to know Qt has built-in way for finding that out, albeit for some reasons getting |
|
@sem-geologist macOS does some shenanigans here, and manipulates colors that it thinks the application has specified to be inline with system defaults, to make them appear to match the rest of the system theme; and I believe that's what's happening with some of the transparent backgrounds for some of the widgets (which is why we see labels written twice for example). Annoyingly, this means you need to be on macOS to replicate the issue. @Wubbzi thanks for the PR, I'll hop on your branch and do some testing on my machine ...it might take me a day or two. |
|
@sem-geologist Are you sure you're running the code from the PR? The pale lavender color in the outermost groups was part of the code that was replaced. I'd assume that you should only see black or white, seeing as how the text color should be using the TextRole from the application palette. Setting the text color to the TextRole is redundant, I only defined the color and left the code for The darkMode property is pretty nifty, and its actually part of pg. You can find the code in |
|
oh... I see... I am an idiot - had cloned Your tree, and had not changed the branch 🤦. I am leaving the wrong pictures uploaded above (will comment that are old before this PR), as now I see a very clear difference. These changes preserves readability, but looks much better IMHO, in particularly on the dark theme. |
|
@sem-geologist Thank you for taking the time to check - I was beginning to worry that macOS had gotten the best of me(again). |
This problem also shows on non-mac platforms with really long action names. The label will 'leak' out from underneath the button. I was able to solve this in my application with I was trying to find a reference to someone's message in the discord sprint chat but I'm not sure how to search the archived chat |
I'll ask the mods there if we can get the archive. |
|
Here was the message I remembered seeing:
I think this change would work better just in the override of widgets that don't have a name column, rather than at the ParameterItem level. So I might still recommend the In either case, I think it's fair to say this is outside the scope of the palette PR (unless you want to include it 🙂) (i.e. A transparent button should be fine provided there is no text behind it) |
|
It's not working on qdarkstyle.load_stylesheet (a use case for several users, there's a dedicated pyqtgraph section in the repo. Is that something that can be changed in the palette here, or something qdarkstyle should alter?
|
|
ooooo nice call on checking against qdarkstyle; we may need to get some input from them on how to "best" go about this. |
|
@ntjess wait... does qdarkstyle render this PR differently from master? If not, there is likely nothing that we can do on our end. |
Which OS/Qt version are you using in the dark screenshot for this PR? I'm not sure why the |
|
I seem to remember that qdarkstyle just restyles everything by stylesheet, and does not even try to touch the QPalette. |
|
It looks like qdarkstyle indeed is limited to the stylesheet with no palette integrations: https://github.com/ColinDuquesnoy/QDarkStyleSheet/blob/26c3878fff742c535058bf5d08643d0135cde2af/qdarkstyle/__init__.py#L275 @Wubbzi I'm not sure about how to load official dark themes, so I used an unofficial gist here: https://gist.github.com/lschmierer/443b8e21ad93e2a2d7eb Running on Zorin OS 16 with PyQt5 5.15.2. List of recognized styles just shows "Fusion" and "Windows". Thus, I'm not confident if my representation is truly indicative of a dark style. |
| altBackgroundColor = palette.dark().color() | ||
|
|
||
| # if stylesheet and 'QDarkStyleSheet' in stylesheet: | ||
| if app.styleSheet(): # Assume the stylesheet is qdarkstyle |
There was a problem hiding this comment.
Thanks for checking for a stylesheet --I recommend looking for "qdarkstyle" explicitly in the sheet, rather than just checking it is nonzero in length. The header of qdarkstyle's sheet is
/* ---------------------------------------------------------------------------
WARNING! File created programmatically. All changes made in this file will be lost!
Created by the qtsass compiler v0.3.0
The definitions are in the "qdarkstyle.qss._styles.scss" module
--------------------------------------------------------------------------- */
/* Light Style - QDarkStyleSheet ------------------------------------------ */
/*
See Qt documentation:
So there are a few magic strings you can check for
There was a problem hiding this comment.
With load_stylesheet(palette=LightPalette), it looks a bit wonky:
Note that "DarkStyle" is commented out in this case as opposed to Light Style
There was a problem hiding this comment.
Hows this? I feel like the text could be better on the outermost groups.. maybe not quite so dark or something with color.
|
Is there a way to extract the color code programmatically from the style
sheet that's applied?
…On Tue, Nov 16, 2021 at 19:18 ntjess ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pyqtgraph/parametertree/parameterTypes/basetypes.py
<#2101 (comment)>:
> - self.setBackground(c, QtGui.QBrush(QtGui.QColor(100,100,100)))
- self.setForeground(c, QtGui.QBrush(QtGui.QColor(220,220,255)))
- font = self.font(c)
- font.setBold(True)
+ """
+ Change item's appearance based on its depth in the tree
+ This allows highest-level groups to be displayed more prominently.
+ """
+ app = QtWidgets.QApplication.instance()
+ palette = app.palette()
+ textColor = palette.text().color()
+ backgroundColor = palette.button().color().darker(110)
+ altBackgroundColor = palette.dark().color()
+
+ # if stylesheet and 'QDarkStyleSheet' in stylesheet:
+ if app.styleSheet(): # Assume the stylesheet is qdarkstyle
Thanks for checking for a stylesheet --I recommend looking for
"qdarkstyle" explicitly in the sheet, rather than just checking it is
nonzero in length. The header of qdarkstyle's sheet is
/* ---------------------------------------------------------------------------
WARNING! File created programmatically. All changes made in this file will be lost!
Created by the qtsass compiler v0.3.0
The definitions are in the "qdarkstyle.qss._styles.scss" module
--------------------------------------------------------------------------- */
/* Light Style - QDarkStyleSheet ------------------------------------------ */
/*
See Qt documentation:
So there are a few magic strings you can check for
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2101 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE5Z7XHNQKFECL5VZ4F4L3UMMNHZANCNFSM5IA5G6BQ>
.
|
|
Sorry if it isn't helpful, but I've been running a QPalette like this for darkmode on windows. It isn't perfect yet (and the color definitions need some cleanup), but it kind of matches qdarkstyle in colors. Would it make sense to apply that manually if a desire for dark mode is detected on Windows? # QDarkstyle compatible UI colors
'DS_bg_light' : '#505F69', 'DS_bg_normal' : '#32414B', 'DS_bg_dark' : '#19232D',
'DS_fg_light' : '#F0F0F0', 'DS_fg_normal' : '#AAAAAA', 'DS_fg_dark' : '#787878',
'DS_sel_light': '#148CD2', 'DS_sel_normal': '#1464A0', 'DS_sel_dark': '#14506E',
def dark_QPalette():
# color definitions roughly match QDarkstyle
BLACK = QColor('#000000')
BG_LIGHT = QColor('#505F69')
BG_NORMAL = QColor('#32414B')
BG_DARK = QColor('#19232D')
FG_LIGHT = QColor('#F0F0F0')
FG_NORMAL = QColor('#AAAAAA')
FG_DARK = QColor('#787878')
SEL_LIGHT = QColor('#148CD2')
SEL_NORMAL = QColor('#1464A0')
# SEL_DARK = QColor('#14506E')
qpal = QPalette( QColor(BG_DARK) )
for ptype in ( QPalette.Active, QPalette.Inactive ):
qpal.setColor( ptype, QPalette.Window , QColor( COLORS['DS_bg_normal'] ) )
qpal.setColor( ptype, QPalette.WindowText, QColor( COLORS['DS_fg_light' ] ) )
qpal.setColor( ptype, QPalette.Base , QColor( COLORS['DS_bg_dark' ] ) )
qpal.setColor( ptype, QPalette.Text, FG_LIGHT )
qpal.setColor( ptype, QPalette.AlternateBase, BG_DARK )
qpal.setColor( ptype, QPalette.ToolTipBase, BG_LIGHT )
qpal.setColor( ptype, QPalette.ToolTipText, FG_LIGHT )
qpal.setColor( ptype, QPalette.Button, BG_DARK )
qpal.setColor( ptype, QPalette.ButtonText, FG_LIGHT )
qpal.setColor( ptype, QPalette.Link, SEL_NORMAL )
qpal.setColor( ptype, QPalette.LinkVisited, FG_NORMAL )
qpal.setColor( ptype, QPalette.Highlight, SEL_LIGHT )
qpal.setColor( ptype, QPalette.HighlightedText, BLACK )
qpal.setColor( QPalette.Disabled, QPalette.Button, BG_NORMAL )
qpal.setColor( QPalette.Disabled, QPalette.ButtonText, FG_DARK )
qpal.setColor( QPalette.Disabled, QPalette.WindowText, FG_DARK )
return qpal |
|
@NilsNemitz Your solution is more robust than that gist -- thanks. Here is the new screenshot, everything appears to be in order
|
We could extract the entire style sheet with If we changed the style sheet for any widget we would have to replace everything(margin,border,etc) not just the colors. I do have some old code that was used to do something very similar that could be adapted if needed. @NilsNemitz I can't test this right now, but I think we could use the current method for windows although it might need to be tweaked. def onPaletteChange(palette):
color = palette.base().color().name()
app = QtWidgets.QApplication.instance()
app.setProperty('darkMode', color.lower() != "#ffffff")@ntjess Originally i was checking for 'QDarkStyleSheet' at the top of the style sheet but I wasn't sure how many groups one of these trees might have in the real world and I was concerned about slowing down the startup with all of the |
|
Does |
|
Really glad you asked about this! When I was looking for the answer I realized This means we need to run The |
|
This (opens pop-corns slowly) is fun to watch :) . Jokes aside, I think most of well thought themes use no absolute white or absolute black. i.e. Breeze Dark (which I won't hide like a lot) of KDE5: https://develop.kde.org/hig/style/color/dark/ |
That's probably a bag of pain we want no part of. I think going w/ hardcoded palette values that @NilsNemitz detailed above for this PR is the way to go. It seems to look good in the most configurations, which is an improvement over the current implementation. That meets the criteria for merging IMO. I certainly didn't intend for the scope of this to blow up so much, and if we want to revisit proper detection of themes on KDE/Windows/macOS, we can certainly do so, but I don't think this PR is the place to do it. |
Sounds like a plan. I'm thinking that we can check for a qdarkstyle stylesheet(light or dark), if it exists, let the stylesheet take over, if there is no qdarkstyle stylesheet and the darkMode property is set, we apply the palette above to the ParameterTree. Should I keep the lightMode colors from the initial commit in cases where there is no stylesheet and no darkMode? I'll wait until tomorrow afternoon to commit the changes in case there are any objections/suggestions. |
If this check can be done w/o difficulty/complexity; I think that's fine... if users are specifying their own style sheet, they know what they want and would probably serve us to respect that.
While those colors definitely look a bit dark for a light mode theme, those colors are definitely intended to be darker than normal, so I personally think they're fine for their intended purpose. |
… os color theme is changed while the app is running.
|
Hi @Wubbzi I just ran this branch, this worked beautifully 🎊 Handled switch between light/dark mode seamlessly! I would recommend adopting the suggested changes @NilsNemitz regarding Lastly, looks like you're using a python 3.8 feature, Looking at the diff, the |
I've already made these changes to my local branch, I just need to push them to github
Unless the tree has 1000s of groups, the difference with and without the caching isn't really noticeable, so I'm fine with moving it out, or changing it to use functools lru cache, if you'd like have some sort of caching until we're able to use cached_property. |
👍🏻
I'm good w/o caching. |
Added method in `pg/__init__.py` to apply app palette based on user input - meant to be used with QDarkStyle
|
This update includes a light and dark palette meant to be used with a QDarkStyle stylesheet. Either the Palette.ID from the pg.applyPalette(app, qdarkstyle.DarkPalette.ID)
app.setStyleSheet(qdarkstyle.load_stylesheet(palette=qdarkstyle.DarkPalette))
pg.applyPalette(app, 'light')
app.setStyleSheet(qdarkstyle.load_stylesheet(palette=qdarkstyle.LightPalette)) |
pyqtgraph/palette.py
Outdated
| return qpal | ||
|
|
||
|
|
||
| def light_QPalette(): |
There was a problem hiding this comment.
Since we are keeping these private, should we give them more specific names?
getQDarkStyleDarkQPalette() and getQDarkStyleLightQPalette(), maybe?
I think we might want to keep the namespace open for our own version of just "light" and "dark" palettes.
The very dark colors in QDarkStyle do not work well with e.g. black outlines of UI elements and the black shading applied according to button state. It really needs the stylesheet.
I'd ultimately like to have a dark mode palette that works without depending on an external stylesheet, and I would want to refer to that as just "dark". :)
There was a problem hiding this comment.
I'm all for QDarkStyleDarkQPalette() and QDarkStyleLightQPalette() but pep8 doesn't like the java style getters/setter names. We might even want to make these properties.
I'd ultimately like to have a dark mode palette that works without depending on an external stylesheet, and I would want to refer to that as just "dark".
Agree! more about this here
There was a problem hiding this comment.
I think the content of palette.py needs some more thought before we let users play with it directly.
This might better use some internal dictionary that is easy to add to. Ideally, the user should be able to throw in their own palette info into the colors folder and grab it from there in multiple applications.
At this time, the strategy might be to keep everything in here private and marked experimental. applyPalette is the only supported access to this... for now.
There was a problem hiding this comment.
Regarding the naming:
We are basically following the Qt CamelCase naming conventions, at least on the API side. So we regularly deviate from python standards. @j9ac9k has the best overview here, though.
There was a problem hiding this comment.
@Wubbzi we generally deviate from pep8 when it comes to naming convention to public portions of our API as @NilsNemitz suggested. We will go snake_case for methods intended for internal methods, and use the leading underscore to designate if something should be considered private or not.
On top of using camelCase for public API methods; we try and follow Qt naming conventions when feasible (for example we do this with .setData() .getData() and a variety of other methods that we feel are analogous to other parts of the Qt API.
Of course the above are just guidelines. ... regarding applyPalette would it be more appropriate to use .setPalette()? That method is used in other parts of the Qt framework:
EDIT: I looked at the diff and saw you call .setPalette within the method 🤦🏻
There was a problem hiding this comment.
We are basically following the Qt CamelCase naming conventions, at least on the API side
I don't have any problem with the case conventions pg uses. The snake_case/true_property stuff that was added in pyside6 looks so wrong, and I hope it never catches on.
I was trying to stick with the Qt conventions. for example, Qt has setMaximumHeight() but when you want to get it, its just maximumHeight() not getMaximumHeight() I stuck with the capital Q because I was afraid it would look weird. Qt does lowercase their methods that begin with q, like qAbs,qRound but you don't see those used in Python very often.
There was a problem hiding this comment.
@j9ac9k oooh, another dash user!
We will go snake_case for methods intended for internal methods, and use the leading underscore to designate if something should be considered private or not.
Oh, I didn't realize thats what was meant by
the strategy might be to keep everything in here private and marked experimental.
regarding applyPalette would it be more appropriate to use .setPalette()
Yep. I was trying to stick as closely to what Nils suggested as I could, so after I copied it out of chat I never changed it, but it has now been changed.
There was a problem hiding this comment.
Well, my thought process wasn't that sophisticated.
More like "make sure nobody uses it directly, whatever we put now, we'll certainly break it later when we put more functionality there"
There was a problem hiding this comment.
another dash user!
if you're on macOS and a developer and not using Alfred + Dash ...that's a crime 😆
|
|
||
| # when changing from light to dark mode updateDepth(GroupParamItem) will insert what I think is the | ||
| # invisible root node. Most of the time, in the example app the title for this node | ||
| # will be empty, occasionally its named params. |
There was a problem hiding this comment.
"it is name 'params'"?
Since the comment is long already, maybe include what actually happens?
"In this case, we hide it again by setting sizeHint to zero."? (Is that what happens?)
There was a problem hiding this comment.
"it is name 'params'"?
changed it to occasionally the title is 'params'., if that helps to be more clear.
"In this case, we hide it again by setting sizeHint to zero."? (Is that what happens?)
Not exactly, it appears to be hidden by default, unless given a size hint by the code below. I'm making a small change to the code so its easier to tell whats happening.
title = self.param.title()
if not title or title == 'params':
return
self.setText(0, title)
fm = QtGui.QFontMetrics(self.font(0))
textFlags = QtCore.Qt.TextFlag.TextSingleLine
size = fm.size(textFlags, self.text(0))
size.setHeight(int(size.height() * 1.35))
size.setWidth(int(size.width() * 1.15))
self.setSizeHint(0, size)There was a problem hiding this comment.
How about just sticking a comment on the setSizeHint line?
# This makes sure that the label remains visible
...or invisible, because I still don't know what this is actually trying to do :)
There was a problem hiding this comment.
The only thing I've changed here is in this method is:
# This makes sure that items with no title or the title 'params' remain invisible
title = self.param.title()
if not title or title == 'params':
returnIf there isn't a title or the title is ''params'` then we don't want to run the code below to give it a size. It seems to default to 0,0 width/height, but I didn't look very deeply into it because simply checking that the title wasn't empty did what we wanted.
How about just sticking a comment on the setSizeHint line?
This isn't my code, but the code on the size hint line sets a size hint of size for column 0. The size is set to the width/height *1.35 of a single line of whichever font self.font(0) returns
There was a problem hiding this comment.
I maybe missed this bit then
--> # This makes sure that items with no title or the title 'params' remain invisible
That was the thing that wasn't clear to me.
Sorry! It just felt weird that there was a three line comment that seemed to build up to something and then didn't tell me what it was actually doing in the end :)
| @@ -1,11 +1,10 @@ | |||
| import builtins | |||
| from functools import partial | |||
There was a problem hiding this comment.
Yes, its being used here in GroupParameterItem and is being used to always return the original font size: The current font size is returned and incremented by 1 for items where depth=0. Since updateDepth is now run every time(to paint the backgrounds), when switching from dark/light system themes, the font size would increase 1 point, every time the theme is switched without it(or something similar).
def __init__(self, param, depth):
ParameterItem.__init__(self, param, depth)
# prevent 0 depth item's font from growing on every call of updateDepth
self.fontPointSize = partial(self.font(0).pointSize)
There was a problem hiding this comment.
Would it have been the same as self.fontPointSize = self.font(0).pointSize?
There was a problem hiding this comment.
Would it have been the same as self.fontPointSize = self.font(0).pointSize?
The behavior would have been the same. The difference to me is, when an attribute is defined as in __init__, generally you wouldn't expect that its required to be called later. Seeing it defined in a lambda, or with partial lets you know that it is a callable.
There was a problem hiding this comment.
I think this isn't used any more with the most recent commit; once this import is removed, I think we can merge this 🎉
| def __init__(self, param, depth): | ||
| ParameterItem.__init__(self, param, depth) | ||
| # prevent 0 depth item's font from growing on every call of updateDepth | ||
| self.fontPointSize = partial(self.font(0).pointSize) |
There was a problem hiding this comment.
I have no experience with this.
If this defines a fontPointSize method for WidgetParameterItem, maybe the boring, verbose way of going through def fontPointSize might be more readable?
There was a problem hiding this comment.
In this specific case, this is basically the same as self.fontPointSize = lambda: self.font(0).pointSize() but is marginally faster.
I didn't define a method because I didn't want to add any methods to the object that weren't necessary elsewhere in the code.
I think this is pretty readable as is but I'll change this into a property instead.
@ property
def fontPointSize(self):
return self.font(0).pointSize()There was a problem hiding this comment.
Oh, I find that much easier to read :)
"I will give you this if asked for fontPointSize()"
Last question:
This is not solved by simply overriding / providing a matching pointSize() method here?
There was a problem hiding this comment.
I don't think I follow.. If you're asking "Why does this need to be a property, wouldn't a method work just the same?" the answer is yes, it would work but font.setPointSize(self.fontPointSize() + 1) looks weird compared to font.setPointSize(self.fontPointSize + 1)
There was a problem hiding this comment.
This is nitpicky and not a large concern. Overall, we typically don't reach into other objects to grab their properties. Even though we are not Java, it seems that we more commonly go through accessor methods.
I think my viewpoint is
- if this is used internally only (as in your example), then do we even need it over accessing
self.font(0).pointSize()directly where applicable? - if this provides a regular interface for other parts of the code, going through a normal method definition is the easiest to find and document. And it clearly sells "use this to get this information". This would be particularly true if you are basically reporting the effective pointsize for this
ParameterItem. Other objects do that throughpointSize(), wouldn't it be nice to match that?
There was a problem hiding this comment.
if this is used internally only (as in your example), then do we even need it over accessing self.font(0).pointSize() directly where applicable?
Actually, now that I think about it, the method or lambda won't work either.
# we define the font as
font = self.font(0/1)
# we increment based on the current size
font.setPointSize(self.font(0/1).pointSize() + 1) # this will grow when palette is changed.
# same goes for anything that calls the method of self.font().pointSize()
# to get the current size, as its increased every time we set the font
self.setFont(0/1,font)
If we set the values in the __init__
self.fontPointSize = partial(self.font(0).pointSize) # set only once when class initialized
self.fontPointSize = self.font(0).pointSize # set only once when class initialized
# we increment with the same pointSize every time updateDepth is run
font.setPointSize(self.fontPointSize()+1)
# when the font is set it will always use size 14(or whatever the actual size is)
There was a problem hiding this comment.
So the issue then is that self.font(0) returns a copy of (instead of a reference to) a QFont?
From the Qt docs:
QFont QTreeWidgetItem::font(int column) const| h, s, l, a = color.getHslF() | ||
| lightness = 0.5 + (l - 0.5) * .8 | ||
| background = QtGui.QColor.fromHslF(h, s, lightness, a) | ||
| altBackground = color |
There was a problem hiding this comment.
No problem, but you might be able to help future devs to figure this out faster.
What you now call "color" is the default background color.
What you call "background" is a modified background color.
Maybe swapping the names around makes that easier to follow (and saves a line):
background = palette.base().color()
h, s, l, a = background.getHslF()
lightness = 0.5 + (l - 0.5) * .8
altBackground = QtGui.QColor.fromHslF(h, s, lightness, a)There was a problem hiding this comment.
I guess I did it this way because I didn't want to call setBackground(altBackground), pick between using setBackground more than once, or writing an else.
There was a problem hiding this comment.
I think I'd be more confused seeing setBackground(altBackground) and having to trace my way through the code to figure out why it wasn't called with background
| if depth == 0: | ||
| background = altBackground | ||
| font.setPointSize(self.fontPointSize() + 1) | ||
| self.setBackground(c, background) |
There was a problem hiding this comment.
If you swap above, these need to swap, too :)
There was a problem hiding this comment.
Maybe just put
if depth == 0:
font.setPointSize(self.fontPointSize() + 1)
self.setBackground(c, background)
else:
self.setBackground(c, altBackground)`Then it should be clear what background goes where.
pyqtgraph/__init__.py
Outdated
| return c | ||
|
|
||
|
|
||
| def applyPalette(app,name): |
There was a problem hiding this comment.
Needs documentation before merge, but we can work that out when we know that this is the way we want to do things.
(I like it.)
There was a problem hiding this comment.
I'm changing the name parameter to style. Do you think we should set a default where style = qarkstyleDark?
There was a problem hiding this comment.
I think we want people to read the docs on this, so I think it is better not to set a default.
We should accept a QPalette for the style parameter, though, and we should check the base color to update the darkMode flag!
There was a problem hiding this comment.
We should accept a QPalette for the style parameter, though, and we should check the base color to update the darkMode flag!
Do you mean in addition to accepting a string, like this?
def applyPalette(app, style):
if isinstance(style, str):
style = style.lower()
if style == 'qdarkstylelight':
p = palette.getQDarkStyleLightQPalette()
elif style in ['qdarkstyle','qdarkstyledark']:
p = palette.getQDarkStyleDarkQPalette()
else:
raise ValueError(f'no palette by the name {style} exists')
elif isinstance(style, QtGui.QPalette):
p = style
app.setPalette(p)
else:
raise TypeError('style should either be a string or QPalette')
app.setPalette(p)There was a problem hiding this comment.
Yes. Except for not setting the palette twice, maybe.
And with the overall addition of the basecolor lightness check from mkQApp.
That would go just before or after the final setPalette call, when we know that p contains a QPalette.
There was a problem hiding this comment.
Yes. Except for not setting the palette twice, maybe.
I just wanted to make sure it was really set. Like really set...
Fixed!
And with the overall addition of the basecolor lightness check from mkQApp.
That would go just before or after the final setPalette call, when we know that p contains a QPalette.
Gotcha will emit the palette through paletteChanged, though... is the darkMode property used other than the example app?
There was a problem hiding this comment.
Ah, if we catch the change there and set the flag, then we're all good!
Let's assume for now that some part of the code somewhere probably does something with the darkMode flag. So it's nice to always have it up-to-date.
pyqtgraph/__init__.py
Outdated
|
|
||
| def applyPalette(app,name): | ||
| name = name.lower() | ||
| if name == "light": |
There was a problem hiding this comment.
I would like to advertise (in the docs) a name that is specific to QDarkStyle here.
'qdarkstyle' and 'qdarkstylelight' come to mind.
We can default 'dark' and 'light' to these, but we might point these simple names at palettes that do not depend on a stylesheet in the future?
To clarify
if name in ['qdarkstylelight', 'light']:
and
if name in ['qdarkstyle','dark']:
seem more appealing to me.
The docs will only mention the qdarkstyle names for now.
There was a problem hiding this comment.
To avoid confusion in the future about why the 'dark' palette suddenly looks different with the qdarkstyle dark palette stylesheet, should we just stick with names specific to qdarkstyle?
if name == "qdarkstylelight":
...
elif name =="qdarkstyledark"
...
There was a problem hiding this comment.
I think those would be good canonical names.
I would tend towards "qdarkstyle" for the dark style, that seems less redundant.
I wouldn't mind an alias to "qdarkstyledark".
You are probably right about not having a 'dark' label until we think about that some more.
There was a problem hiding this comment.
Okay, the changed to:
if style == 'qdarkstylelight':
p = palette.getQDarkStyleLightQPalette()
elif style in ['qdarkstyle','qdarkstyledark']:
p = palette.getQDarkStyleDarkQPalette()
|
|
||
| def updatePalette(self): | ||
| """ | ||
| - called when application palette changes - |
There was a problem hiding this comment.
Pretty sure that this is what makes sphinx confused, and that
"Called when the application palette changes"
would not cause any trouble.
This partially addresses #1958
These changes will still need to be tested on windows/Linux.
Group title colors were changed to the color of QPalette.TextRole but it could easily be changed back to the original in dark mode.
Light
Dark