Conversation
change some mistake
2452581 to
674c2a1
Compare
|
Hi @aksy2512, It solves the problem partly ;) Another remark on formatting: |
|
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. |
|
@j9ac9k I have rewritten the code and used "for" loop instead of the if statements. Please have a look. |
|
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 :) |
|
@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. |
|
This PR was superseeded by #2799 |
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
