Skip to content

Change viewBox.py#2540

Closed
aksy2512 wants to merge 2 commits intopyqtgraph:masterfrom
aksy2512:my-new-feature
Closed

Change viewBox.py#2540
aksy2512 wants to merge 2 commits intopyqtgraph:masterfrom
aksy2512:my-new-feature

Conversation

@aksy2512
Copy link
Copy Markdown
Contributor

Detail the reasoning behind the code change. If there is an associated issue that this PR will resolve add

Fixes #2415

I wrote the following code inside the updateViewRange function
image

change some mistake
@bbc131
Copy link
Copy Markdown
Contributor

bbc131 commented Dec 16, 2022

Hi @aksy2512,
I just had a look at your PR and also tested it quickly.

It solves the problem partly ;)
I took my example from #2415 and observe the following:
Making the region larger correctly triggers the right plot at some point to increase its x-range.
But if you make the region than smaller again, this will decrease the right plots x-range also! This definitely shouldn't be the case.
I just looked around in the updateViewRange method and saw, that one might have to adjust self.state['targetRange']?!
I hadn't the time to look closer into it, this is just a wild guess.

Another remark on formatting:
I suggest you to use black [1] to get a nice formatting.
For example I don't like the brackets at the "if"s and black would also normalize all whitespaces.
[1] https://pypi.org/project/black/

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Dec 20, 2022

Hi @aksy2512

Sorry I haven't reviewed this yet. I had a long stint of being sick and this time of year it's difficult to allocate time for pyqtgraph work due to family time and so on.

generally speaking I'm not big on a bunch of if-statements like this, but if it works it works. ViewBox.py is a very sensitive to changes so even though the implementation looks right, I'm going to try and break this a few times.

I am fairly confident this PR (or a variant of this PR that fixes the same issue) will be merged before the next release.

@aksy2512
Copy link
Copy Markdown
Contributor Author

@j9ac9k I have rewritten the code and used "for" loop instead of the if statements. Please have a look.
Thanks

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jan 19, 2023

Hi @aksy2512

Sorry for the really long wait for feedback... ViewBox.py is among the most fragile parts of the library, so naturally I gravitate towards avoiding changes to it unless absolutely necessary.

There is an issue I'm a bit hung up on that I posted in the channel, why should the second limit be adjusted and not the first limit as this PR does.

I was going to suggest adding a test, but I can see we already skip the viewbox limits/resize test already as it's not working, so that would not be a reasonable request for a newcomer to the library :)

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Jan 20, 2023

@bbc131 made a good point in this post here #2415 (comment)

They point out that the current logic in this PR makes no attempt to compare the computed upper bound of the range against xMax/yMax, so there could be a situation where the newly computed upper bound is outside of the specified limits. If that occurs, the lower bounds should likely be adjusted when the upper bound is placed at the xMax or yMax limit.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Mar 26, 2025

This PR was superseeded by #2799

@j9ac9k j9ac9k closed this Mar 26, 2025
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.

ViewBox.setLimits() doesn't trigger update if view range not within new limits

3 participants