WIP Adapted Qt-relevant files to let Qt version work with the new AbstractDiffractometer by rhfogh · Pull Request #1659 · mxcube/mxcubecore

rhfogh · 2026-08-28T11:19:00Z

The main work is by Martin Savko, making QtGraphicsManager a subtype of SampleView - which should impact the Qt version only. There are other adjustments, and some GPhL-specific fixes. The version is smoke-tested with both the Qt and web version in mock mode.

marcus-oscarsson · 2026-09-02T13:54:25Z


    logging.getLogger("HWR").debug("X=%s,Y=%s", X, Y)
-    chi_angle = math.radians(chi_angle)
+    chi_angle = math.radians(chi_angle) if chi_angle else 0.


Dont think you need if chi_angle else 0 as you use a kwarg for chi_angle above ?

marcus-oscarsson · 2026-09-02T13:58:39Z

-            sampx.motor: float(sampx.get_value() + sampx.direction * dx),
-            sampy.motor: float(sampy.get_value() + sampy.direction * dy),
-            phiz.motor: (
+            sampx: float(sampx.get_value() + sampx.direction * dx),


Not sure about this one. The previous code used a Motor object as a key while this code would use CentringMotor object. Would that still be compatible with the calling code ? Neither is good practice but I don't think this is the good moment to change that ?

Agree with @marcus-oscarsson

marcus-oscarsson · 2026-09-02T13:59:07Z

        pixels_per_mm = diffr.get_pixels_per_mm()
        diffr.wait_status_ready(5)
-
+        print(f"self.centring_motors {self.centring_motors}")


Please Replace with log if the print is needed

Or remove it.

marcus-oscarsson · 2026-09-02T14:01:50Z

-                "centringFailed",
-                self.diffractometer_centring_failed,
-            )
+            #self.connect(


I think this could or most likely will break a number of things ?

marcus-oscarsson · 2026-09-02T14:03:10Z

-        self.emit("centringAccepted", (True, self.get_centring_status()))
+        args = (True, self.get_centring_status())
+        self.emit("centringAccepted", args)
+        self.create_centring_point(*args)


I think this should be done in the handler that listens to centringAccepted

Indeed only the signal emit is needed here.

marcus-oscarsson · 2026-09-02T14:08:26Z

Looks good overall,

I'm concerned about the changes made to sample_centring.py
and SampleView.py. I think there are some changes that are a bit risky.

sample_centring - I'm not certain about the impact of removing .motor
SampleView - Addition of self.create_centring_point(*args)

rhfogh · 2026-09-02T14:14:40Z

@marcus-oscarsson Thanks for checking. I have changed this to WIP, and I think we need to wait for Martin Savko's input. He was the one who did the changes, and I had done a quick smoke test, found it worked, and made the PR. Maybe that was a mistake. Anyway with some of your comments I think we had better hear from Martin either why this is all OK, or that he does not think this is yet ready for merging and what might need doing.
My apologies for jumping the gun.

marcus-oscarsson · 2026-09-02T14:55:08Z

No problem, its of course very much appreciated that both of you would like to advance. You did well :). We are using both SampleView and sample_centring. I guess others are as well at-least SampleView so its better to a bit cautious. I think your PR on mxcubeweb can still be merged there did not seem to be direct link ?

MotorMockup.py: default direction is 1 sample_centring.py: motor.motor -> motor e.g. sampx.motor -> sampx queue_model_objects.py: call set_diffractometer_motor_names(names) in __init__()

comment out lights for now in the config file for now.

beteva

Dear @rhfogh, thanks for doing the adaptation of the Qt. Most of it is fine, just few comments:
The GenericDiffractometer should not be involved if you go for AbstractDiffractometer.
Some of the changes in SampleView will actually break it.

beteva · 2026-09-02T15:08:17Z

+    def accept_centring(self):
+        return True
+
+    def user_confirms_centring(self):
+        return False


Everything concerning the centring is now in SampleView.

Suggested change

def accept_centring(self):

return True

def user_confirms_centring(self):

return False

beteva · 2026-09-02T15:09:08Z

+
+    def use_sample_changer(self):
+        return True
+


Is this really needed?

beteva · 2026-09-02T15:11:19Z


    CENTRING_MOTORS_NAME = [
-        "phi",
+        "omega",


GenericDiffractometer should be replaced by AbstractDiffractometer.

beteva · 2026-09-02T15:13:44Z

-        for key, val in self.centring_motors.items():
-            motors_dict.update({key: val.motor.get_value()})
+        motors_dict = dict(
+            (key, val.get_value()) for key, val in self.centring_motors.items()


Suggested change

(key, val.get_value()) for key, val in self.centring_motors.items()

(key, val.motor.get_value()) for key, val in self.centring_motors.items()

Do you really need to change this for the sake of a one line less? 😄

beteva · 2026-09-02T15:15:44Z

        pixels_per_mm = diffr.get_pixels_per_mm()
        diffr.wait_status_ready(5)
-
+        print(f"self.centring_motors {self.centring_motors}")


Or remove it.

beteva · 2026-09-02T15:17:13Z

-        self.emit("centringAccepted", (True, self.get_centring_status()))
+        args = (True, self.get_centring_status())
+        self.emit("centringAccepted", args)
+        self.create_centring_point(*args)


Indeed only the signal emit is needed here.

beteva · 2026-09-02T15:18:27Z

+        args = (False, self.get_centring_status())
+        self.emit("centringAccepted", args)


What was the reason to define args?

beteva · 2026-09-02T15:21:49Z

        for mot in self.motors_hwobj_dict.values():
            mot.set_value(random.uniform(0.0, 8.8))
-        self.omega.set_value(random.uniform(0, 359.9))
+        self.omega.set_value(random.uniform(0, 360))


beteva · 2026-09-02T15:22:56Z

+
+    def get_image_scale_list(self):
+        return [1]


Could you, please, tell us what is this used for?

rhfogh · 2026-09-02T15:36:43Z

@beteva As I said to Marcus, this is (almost) all Martin's work, and it is possible that I went with an incomplete version. I take your comments on board, but I think we need Martin to react to them first before I start making changes.

marcus-oscarsson reviewed Sep 2, 2026

View reviewed changes

rhfogh changed the title ~~Adapted Qt-relevant files to let Qt version work with the new AbstractDiffractometer~~ WIP Adapted Qt-relevant files to let Qt version work with the new AbstractDiffractometer Sep 2, 2026

rhfogh added the wip Work in progress label Sep 2, 2026

rhfogh assigned MartinSavko Sep 2, 2026

Martin Savko and others added 8 commits September 2, 2026 15:59

Catching also KeyError

5618cd5

Debug centring in mxcubeqt mockup.

0c6dc16

MotorMockup.py: default direction is 1 sample_centring.py: motor.motor -> motor e.g. sampx.motor -> sampx queue_model_objects.py: call set_diffractometer_motor_names(names) in __init__()

QtGraphicsManager inherits from SampleView

e09afdf

Adding expected methods to DiffractometerMockup

524b6e7

comment out lights for now in the config file for now.

Minor Python fixes for abstractdiff Qt incorporation

26fcd46

Configuration fixes for abstractdiff Qt incorporation

7384aab

Fixed formatting to pacify linter

37d9d5a

Further fixed formatting to pacify linter

981be73

rhfogh force-pushed the rhfogh_develop branch from 822ecec to 981be73 Compare September 2, 2026 15:02

rhfogh added 2 commits September 2, 2026 16:22

Style fixes for linter

741b986

More style fixes for linter

63ee15d

beteva requested changes Sep 2, 2026

View reviewed changes

	(key, val.get_value()) for key, val in self.centring_motors.items()
	(key, val.motor.get_value()) for key, val in self.centring_motors.items()

		args = (False, self.get_centring_status())
		self.emit("centringAccepted", args)

Uh oh!

Conversation

rhfogh commented Aug 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-oscarsson commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhfogh commented Sep 2, 2026

Uh oh!

marcus-oscarsson commented Sep 2, 2026

Uh oh!

beteva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhfogh commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcus-oscarsson commented Sep 2, 2026 •

edited

Loading

beteva left a comment •

edited

Loading