Plan window improvement #311
Conversation
|
do not review this PR yet, I wanted to add #329 in this PR too, @canismarko |
|
This numpy error is fixed in another PR: a823595 The rest of the tests pass so I'll just review it as is. |
src/firefly/plans/grid_scan.py
Outdated
| self.ui.spinBox_repeat_scan_num.valueChanged.connect(self.update_total_time) | ||
| # Connect scan points change to update total time | ||
| for region in self.regions: | ||
| region.scan_pts_spin_box.valueChanged.connect(self.update_total_time) | ||
| self.ui.relative_scan_checkbox.stateChanged.connect(self.change_background) | ||
|
|
||
| def change_background(self, state): |
There was a problem hiding this comment.
Can this method be moved to the RegionsDisplay class so we don't need to write it twice? It looks identical between the line scan and grid scan versions.
src/firefly/plans/line_scan.py
Outdated
| @@ -12,6 +13,7 @@ | |||
|
|
|||
|
|
|||
| class LineScanRegion(regions_display.RegionBase): | |||
| update_step_signal = Signal(int) | |||
There was a problem hiding this comment.
What is this signal for? It looks like LineScanDisplay.update_regions_step_size() emits this signal, but all it does is connect directly to LineScanRegion.update_step_size(). Could LineScanDisplay.update_regions_step_size() call directly LineScanRegion.update_step_size()?
If we really need a signal, can it be renamed to describe what actually happened? Signals typically represent some changed state in the application (e.g. "checked", "textChanged" etc) instead of an action to take (that's a slot).
There was a problem hiding this comment.
Good idea, fixed, see the changed codes
src/firefly/plans/line_scan.py
Outdated
| # Connect scan_pts_spin_box value change to regions | ||
| self.ui.scan_pts_spin_box.valueChanged.connect(self.update_regions_step_size) | ||
|
|
||
| def update_regions_step_size(self, value): |
There was a problem hiding this comment.
"value" is a bit vague. How about step_size or something like that?
| assert region_0.step_size_line_edit.text() == "N/A" | ||
|
|
||
| # Test edge case: num_points = 1 for Region 1 | ||
| region_1.scan_pts_spin_box.setValue(1) |
There was a problem hiding this comment.
Good call testing for these edge cases!
Things to do before merging: