-
Notifications
You must be signed in to change notification settings - Fork 47
update LaserAutofocusSettingWidget #160
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
Conversation
| def update_threshold_settings(self): | ||
| updates = { | ||
| "laser_af_averaging_n": int(self.spinboxes["laser_af_averaging_n"].value()), | ||
| "displacement_success_window_um": self.spinboxes["displacement_success_window_um"].value(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do any data validation on these? Or is it safe to trust that the spinboxes will do the validation for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the spinboxes will do the validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if user enter 0 or negative value?
software/control/gui_hcs.py
Outdated
| for i in range(1, self.imageDisplayTabs.count()): | ||
| widget = self.imageDisplayTabs.widget(i) | ||
| self.imageDisplayTabs.setTabEnabled(i, not self.performance_mode) | ||
| if self.imageDisplayTabs.tabText(i) != "Laser-Based Focus": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the bare string here is fragile - if someone changes the name of the tab, then this breaks. A safer and easier to maintain way to do this is to pull the tab name out into a class variable. For instance (details missing - but this'll give you an idea):
class HighContentScreeningGui(QMainWindow):
LASER_BASED_FOCUS_TAB_NAME = "Laser-Based Focus"
def setupImageDisplayTabs(self):
self.imageDisplayTabs.addTab(laserfocus_dockArea, HighContentScreeningGui.LASER_BASED_FOCUS_TAB_NAME)
def toggleNapariTabs(self):
# ...
if self.imageDisplayTabs.tabText(i) != HighContentScreeningGui.LASER_BASED_FOCUS_TAB_NAME:
# ...
Alternatively, and even better, we wouldn't use the tab name (since that is really just for human interaction, and can change too easily. For instance - if someone adds a feature that changes the tab name based on context, this will still break).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good identifier for the tabs? I think index may also change.
Uh oh!
There was an error while loading. Please reload this page.