Add a few ImageView improvements#1828
Conversation
+corrected buggy play()
Conflicts: pyqtgraph/imageview/ImageView.py - keyPressedEvent same-line changes
Conflicts: pyqtgraph/imageview/ImageView.py - nframes v. Key.*
|
@outofculture is this ready for a review or were there still some things you wanted to work on? |
|
@ixjlyons argh, no, it's not ready. I was testing things and ran into some problems that didn't have a clear solution, and I haven't come back to it, and now I'll have to start from scratch to figure out what those problems were. I might pare this down again in the process, but I'll put this back on the top of my pyqtgraph stack. |
Conflicts: pyqtgraph/imageview/ImageView.py - imports, same fix twice tests/imageview/test_imageview.py - eof conflict
|
Okay, I finally got back to this and cleaned it up. The test is saner now, I cleaned up a few more docstrings, and I made sure pause/unpause would restore the previous play speed. This is ready for review. |
| self.currentIndex = ind | ||
| self.updateImage() | ||
| if self.discreteTimeLine: | ||
| with fn.SignalBlock(self.timeLine.sigPositionChanged, self.timeLineChanged): |
There was a problem hiding this comment.
nice use of SignalBlock probably should use it more in this library!
pyqtgraph/imageview/ImageView.py
Outdated
| if not self.hasTimeAxis(): | ||
| return (0,0) | ||
| return 0, 0 |
There was a problem hiding this comment.
nit-pick, but for type-correctness, probably should return 0, 0.0
tests/imageview/test_imageview.py
Outdated
| ind, val = iv.timeIndex(iv.timeLine) | ||
| assert ind == count // 2 | ||
| assert val == 0.5 | ||
| iv.togglePause() |
There was a problem hiding this comment.
perhaps we should add a comment here indicating that this call to togglePause should get playback going?
|
Hi @outofculture I did a look-through, looks good to me. I just got nit-picky about the float vs. int return type and requested a comment in the test function, I think this is ready for merging otherwise! Thanks for your work on this! |
NEW features in ImageView: * timeline mouse interactions can be configured as discrete * histograms can be given a title * pause/unpause maintains playback speed Also, some style improvements. Squashed history: * +corrected wrong .opts key which caused example to crash +corrected buggy play() * removes 'pass' * ignorePlaying replaced with ignoreTimeLine * no thanks, import star * use tVals if present; use SignalBlock * small improvements * whitespace * remove discreteTimeLine from opts; remove opts * space toggles playitude * show off new features in example * nframes can be zero; no need to normalize first * bugs in setHistogramPrintView, so just remove it for now * docstring the new methods; lint * INCOMPLETE: test the discrete timeline * fix ui_template * numpy-style docstrings * numpy-style docstrings * fix test fails: imageItem might be None * a linspace across 31 points gives the values I expect * playback should resume at the previous play speed * style: no property decorators unless necessary * int, float; comments to track play state Co-authored-by: Karl Bedrich <karl@bedrich.de> Co-authored-by: Ogi Moore <ognyan.moore@gmail.com>
Taking up #396 (thanks, @radjkarl!), this PR does a little more cleanup. Some of the features were incomplete or had bugs. I fixed some of these, omitted others. Here's what happened to everything:
setHistogramLabellooks good and is connected to the example now.discreteTimeLine, now an init argument, controls snap-to-frame in the timeline. This can now handle non-integer xvals. The example now uses this option. A test now checks this.nframeshandles zero frames now. A test now checks this.play,keyPressEventandtimeIndexmethods are working smoothly. Spacebar now toggles play-through, instead of only starting it. Example now plays.optsneeded additional documentation and scaffolding, and I didn't want to put in the work for what didn't feel particularly impactful.setHistogramPrintViewneeded some improvement, but I didn't understand the expected behavior well enough to take it over, so I just removed it. If this is still valuable to someone, a version with a more thoroughly described spec would be an acceptable addition.