Skip to content
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

Plan window improvement #353

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Plan window improvement #353

wants to merge 14 commits into from

Conversation

Cathyhjj
Copy link
Collaborator

Modified ui for plan windows, and made one more parent class that applied for plan windows not having changeable regions, so to reduce redundant codes

Things to do before merging:

  • add tests
  • write docs
  • update iconfig_testing.toml
  • flake8, black, and isort
  • test on beamline (just by adding to haven-dev queue server, haven't tested whether it runs well)

There is one test I added in the test_count_window test_time_calculator, I don't know why it couldn't pass; I copied it from test_line_scan_plan, which passed; I could not figure out why this did not work.

@Cathyhjj Cathyhjj requested a review from canismarko January 24, 2025 02:13
@@ -43,7 +46,11 @@ def setup_ui(self):
self.start_line_edit.textChanged.connect(self.update_step_size)
self.stop_line_edit.textChanged.connect(self.update_step_size)

def update_step_size(self, num_points=None):
def set_num_points(self, num_points):
self.num_points = max(2, int(num_points)) # Ensure num_points is >= 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I type "1" in scan_pts_spin_box, this line will actually use 2 internally. What does the spinbox show in that scenario? Does it still show "1" or does it correctly update to "2"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to make num_points be a property that reads from the spin box and converts to an integer instead of having to keep track of this state in two places.


def customize_ui(self):
self.ui.run_button.clicked.connect(self.queue_plan)
if hasattr(self.ui, "spinBox_repeat_scan_num"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this belongs in the individual child classes instead of the parent classes. Having a parent class make decisions based on things that might be done by the child classes can cause problems.

For example, what if the child class wants to use this spin box differently, or call it something else, or it's not actually a Qt Signal? The child class has no way to do this without breaking the inheritance chain for this method.

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.

2 participants