Skip to content

Use GroupParameter instead of popup button for PenParameter#2086

Merged
j9ac9k merged 5 commits intopyqtgraph:masterfrom
ntjess:fix-pen-param
Dec 11, 2021
Merged

Use GroupParameter instead of popup button for PenParameter#2086
j9ac9k merged 5 commits intopyqtgraph:masterfrom
ntjess:fix-pen-param

Conversation

@ntjess
Copy link
Copy Markdown
Contributor

@ntjess ntjess commented Nov 14, 2021

No other parameters use a popup system to set their values (I guess a color picker does...). After using the pen parameter a bit, I was frustrated at having to click at least three times (open box, make change, press "ok") just to change a value. This remedies both issues.

Pen parameters are now group parameter children. Everything else is now as normal.
Oh, and the default button respects true pen defaults now, too

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Nov 14, 2021

As yet another bonus, there's so much less misdirection/obfuscation between where the pen gets updates.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 14, 2021

Nice! one of the things I was worried that would be missing out was that the button had a nice representation of what the pen looked like, but you preserved that 👍🏻

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 14, 2021

Only question I have is if we want to move the pen label into the widgets portion of the library; and import it in the parameter type. I know we talked about this earlier, ... I like the idea of parameter types using the widgets within the library, but I do think there is value in making those widgets accessible via pyqtgraph.widgets as well.

If I remember our discussion earlier on this, you were concerned we might run into a case with circular imports, would that be a concern here, as the modified QLabel wouldn't need to know anything about the parameter type (the parameter type would need to know about the QLabel though!), so is circular imports really a concern here?

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Nov 15, 2021

@j9ac9k good point, it should be updated now.

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Nov 15, 2021

Now pen preview shows "changing" details and a delay timer is added similar to #209. The video somehow uploaded with terrible frame rate, but you are welcome to test it out. A 'cosmetic' indicator was also added to the preview label since the pen preview looks the same in both cases without it

LTyVt7MweH.mp4

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 18, 2021

While playing around with this, noticed one issue...and by playing around with it, I mean running

python pyqtgraph/examples/PlotSpeedTest.py

Open the Color Selection Dialog button, select a color (but don't click ok); you'll notice the plot changes color. If you cancel out, it will eventually change back to what it was. I'm not sure this behavior is a deal breaker or not, or is that an issue with ColorButton?

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Nov 18, 2021

I somehow mentally blocked the fact that color picker is in fact a popup. Side note -- should it be a subitem like calendar instead of its own dialog for the same reason as the gripe in this PR? The reason a child relationship makes sense for the pen is because there are few enough editable options, but a color picker needs lots of real estate.

To answer your question though, I listen for a delay in sigValueChanging signals. Since the color picker parameter doesn't expose an API for whether the window is still open, that's why the pen value changes (I assume it's after you wait 1s?)

What if instead of a SignalProxy, I waited until no pen children had focus? This might be akin to a editingFinished signal fired by text inputs. I originally thought of a delay proxy as opposed to immediate updates, but now I think it might make sense for the user to click out of the parameter children (or press some key?) rather than imposing a delay. Or both? That's how a spinbox does it.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 18, 2021

I somehow mentally blocked the fact that color picker is in fact a popup. Side note -- should it be a subitem like calendar instead of its own dialog for the same reason as the gripe in this PR? The reason a child relationship makes sense for the pen is because there are few enough editable options, but a color picker needs lots of real estate.

I think having the color button be a pop up dialog is fine. I could see a case to be made for having a text input that runs the text into mkColor; but that's waaaaaay outside the scope of this PR IMO.

What if instead of a SignalProxy, I waited until no pen children had focus? This might be akin to a editingFinished signal fired by text inputs. I originally thought of a delay proxy as opposed to immediate updates, but now I think it might make sense for the user to click out of the parameter children (or press some key?) rather than imposing a delay. Or both? That's how a spinbox does it.

I think "both" here would be good. Widget loses focus, emit signal right away; otherwise wait the delayed 1s. I don't have a good sense of what kind of code complexity that would add tho.

- Better internal variable names
- Use 'valueChanging' instead of 'changed' logic
- Show 'changing' instead of 'changed' values
- Add 'C' indicator for cosmetic pen
@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Dec 10, 2021

QTreeWidgetItem doesn't seem to have a focus event ecosystem, so I will leave that off the table for this release

@j9ac9k j9ac9k merged commit b73fefa into pyqtgraph:master Dec 11, 2021
@ntjess ntjess deleted the fix-pen-param branch January 9, 2022 15:35
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.

2 participants