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

PlotWidget: Added sigBackendChanged #3890

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Conversation

vallsv
Copy link
Contributor

@vallsv vallsv commented Jun 27, 2023

Added sigBackendChanged to PlotWidget

Related to #3876

Closes #3883

Changelog:

  • Added sigBackendChanged to PlotWidget

@vallsv vallsv requested a review from t20100 June 27, 2023 11:39
@vallsv vallsv self-assigned this Jun 27, 2023
@vallsv
Copy link
Contributor Author

vallsv commented Jun 28, 2023

I was thinking of added current, previous params to the signal.

Usually if you don't do that, the user of the API have to store the backend reference on its side. So i think it could be done without much problem.

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

LGTM

The OpenGLAction can be improved with this

@vallsv
Copy link
Contributor Author

vallsv commented Jun 30, 2023

That's a good idea i have updated the stuff.

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

LGTM!
I removed OpenGLAction.__state since it is no longer used.

@t20100
Copy link
Member

t20100 commented Jul 10, 2023

I was thinking of added current, previous params to the signal.

If it's needed, why not.
It's not Qt-like but some other signals of PlotWidget already provides current, previous params.

But this means you'll give back a previous backend that is no longer used by a PlotWidget, not sure what it's useful for, and risky if used.

@vallsv
Copy link
Contributor Author

vallsv commented Jul 10, 2023

Yes, that is why i am kind of reluctant. In this case we could use 2 signals like aboutToConnect/aboutToDisconnect, but i feel like it's overkill.

@vallsv vallsv merged commit 577d260 into silx-kit:main Jul 10, 2023
@t20100
Copy link
Member

t20100 commented Jul 11, 2023

aboutToConnect/aboutToDisconnect, but i feel like it's overkill.

I agree.

sigBackendChanged can also only provide the current backend, that more Qt-like and does cause trouble with giving access to a disconnected backend.

It's fine as it for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a sigBackendChanged to the PlotWidget
2 participants