Prevent negative clicks during manual centering and display a UI message #1649

alessandro-olivo · 2025-05-15T13:44:43Z

The feature/fix allows handling user clicks that happen while a manual centering step is still in progress. A message is shown on the UI to inform the user.

The issue

After the first manual centering click, if the user clicks again before the first step is completed, a message appears on the UI and the number of available clicks is reduced by one. If the user keeps clicking, the click count can go negative, as shown in the screenshot:

This behavior can be tested adding a time.sleep(3) in the manual_centring method of the DiffractometerMockup HWO

    def manual_centring(self):
        """
        Descript. :
        """
        for click in range(3):
            self.user_clicked_event = AsyncResult()
            #self.waiting_for_click = True
            x, y = self.user_clicked_event.get()
            if click < 2:
+                time.sleep(3)
                self.motor_hwobj_dict["phi"].set_value_relative(90)
        self.last_centred_position[0] = x
        self.last_centred_position[1] = y
        centred_pos_dir = self._get_random_centring_position()
        return centred_pos_dir

The proposed change

In mxcubeweb/core/components/sampleview.py

    def centring_handle_click(self, x, y):
        if HWR.beamline.diffractometer.current_centring_procedure:
+            try:
                HWR.beamline.diffractometer.imageClicked(x, y, x, y)
                self.centring_click()
+            except Exception:
+                return {"clicksLeft": -1}
        else:
            if not self.centring_clicks_left():
                self.centring_reset_click_count()

The change allows, raising an Exception from .imageClicked(...), to inform the frontend that the centering step is still in progress. If the frontend receives a negative click (i.e. "-1") it will show the following message:

Test the feature:

Just apply the following changes in the DiffractometerMockup:

...
+from gevent.lock import Semaphore

from mxcubecore import HardwareRepository as HWR
from mxcubecore.HardwareObjects.GenericDiffractometer import (
    GenericDiffractometer,
    PhaseEnum,
)


class DiffractometerMockup(GenericDiffractometer):
...

    def init(self):
        """
        Descript. :
        """
        # self.image_width = 100
        # self.image_height = 100

        GenericDiffractometer.init(self)
+      self.waiting_for_click = False
+      self.click_lock = Semaphore()
        self.x_calib = 0.000444

...
    
+    def image_clicked(self, x, y, xi=None, yi=None):
+        with self.click_lock:
+            if self.waiting_for_click:
+                self.waiting_for_click = False
+                self.user_clicked_event.set((x, y))
+            else:
+                self.log.warning("User click but the last centring operation"
+                                              " was still in progress")
+                raise RuntimeError("Last centring operation is still in "
+                                                   "progress, click will be ignored")

    def manual_centring(self):
        """
        Descript. :
        """
        for click in range(3):
            self.user_clicked_event = AsyncResult()
+          self.waiting_for_click = True
            x, y = self.user_clicked_event.get()
            if click < 2:
+              time.sleep(3)
                self.motor_hwobj_dict["phi"].set_value_relative(90)

...

Thoughts

Maybe the Exception could be a more specific User-defined Exception ? Should we use user-defined Exceptions in mxcube ?
Is there a better solution ? Maybe with a new signal o something like that ?

axelboc

I'm surprised we hadn't caught this bug sooner. 😱 Good catch! 🎣 It could explain why we were having issues with this in the Cypress tests...

As before, as a safeguard, I would prefer if the back-end returned a 4xx error to the front-end (which would then display that error via showErrorPanel). Then, to improve the user experience, I would find a way to not dispatch the click action while the 3-click centring process is in progress. As you hinted at, this would require a WebSocket message, a Redux action, and a new flag in the state.

marcus-oscarsson · 2025-05-15T14:19:51Z

Very nice :), it would be great with error from the backend as well as @axelboc pointed out.

marcus-oscarsson · 2025-05-16T06:49:17Z

What you suggest is very nice and I think you did an excellent work wit this PR :)

Maybe the Exception could be a more specific User-defined Exception ? Should we use user-defined Exceptions in mxcube ?

Yes, it would perhaps be preferable, or at least to use a more specific exception
As pointed out above this exception could then be handled on the web side and a 400
error could be returned.

Is there a better solution ? Maybe with a new signal o something like that ?

You mean, instead of returning or raising an exception to send a signal and websocket with the result instead ?

That could also work very well. I'm not sure it would make much of a difference for this kind of short interaction.
We are not risking any timeout or similar as each click is handled separately. The logic you suggest above is IMO
somewhat easier to follow and to handle.

axelboc · 2025-05-16T07:07:28Z

I'm not sure it would make much of a difference for this kind of short interaction.

Yeah, you're right, it'd probably be a bit over-engineered. So keeping the 4xx error response in mind, I think that means basically catching the error and showing that "Please wait" message instead of dispatching showErrorPanel, right?

marcus-oscarsson · 2025-05-16T08:07:20Z

Yes, thats what I thought

marcus-oscarsson · 2025-05-19T07:17:14Z

I guess this would also need the necessary changes to MiniDiff and GenericDiffractometer. Have you tested how this behaves without the changes you suggest to GenericDiffractometer ?

alessandro-olivo · 2025-05-19T16:49:57Z

Actually I tried only with DiffractometerMockup, but I can test also for the HWOs you proposed

marcus-oscarsson · 2025-05-27T06:44:16Z

Thanks @alessandro-olivo. The best thing would be if you could add the lock code to the two other diffractometer classes MiniDiff and GenericDiffractometer. I understand that may be complicated as you are perhaps not using them. We will be happy to test the changes to MiniDiff.

It would otherwise be nice if you could test without the changes to DiffractometerMockup so that we are sure that the changes are "backward compatible". If it turns out to be too time contusing to do the changes to MiniDiff, GenericDiffractometer.

Thanks again for the super nice work !

alessandro-olivo · 2025-05-27T12:47:20Z

The best thing would be if you could add the lock code to the two other diffractometer classes MiniDiff and GenericDiffractometer

Right now, the "lock logic" is only implemented in our XRD1Diffractometer HWO, which inherits from GenericDiffractometer but yes, I totally agree with you @marcus-oscarsson , if we added this feature directly to the "generic" class all subclasses should somehow benefit from it.
I was thinking about introducing a new method, maybe something like manual_centring_step() which you have to implement only in the GenericDiffractometer subclasses, or possibly modifying the existing image_clicked() of the GenericDiffractometer method keeping the current behavior if self.click_lock isn’t defined, and use the new "lock logic" only when it is.

In any case, I’ll try to implement this logic in both GenericDiffractometer and MiniDiff HWO as soon as I can, hopefully by the end of this week or early next week.

It would otherwise be nice if you could test without the changes to DiffractometerMockup so that we are sure that the changes are "backward compatible". If it turns out to be too time contusing to do the changes to MiniDiff, GenericDiffractometer.

I've already tested these changes with the DiffractometerMockup as it is (without the "lock logic") and the behavior is the same as the current one, except for the UI message shown when the frontend receives -1 clicks

marcus-oscarsson · 2025-06-05T14:20:16Z

Ok, sounds perfect @alessandro-olivo. So ill merge this for now then !

marcus-oscarsson · 2025-06-05T14:20:43Z

Ah, @alessandro-olivo, could you re-base and then merge please ;)

…feature-diffr_is_moving" in the mxcubecore repository is mandatory for this commit)

…notification in the UI

alessandro-olivo · 2025-06-10T14:04:45Z

Done! I think this week I should be able to give a try to the aforementioned changes

axelboc reviewed May 15, 2025

View reviewed changes

alessandro-olivo added 2 commits June 10, 2025 15:53

Avoid negative clicks counting during centring operation (branch "ao-…

dfae752

…feature-diffr_is_moving" in the mxcubecore repository is mandatory for this commit)

Prevent user clicks during manual centering movements, and display a …

9df2aa2

…notification in the UI

alessandro-olivo force-pushed the ao-feature-avoid_negative_clicks branch from 7120fa8 to 9df2aa2 Compare June 10, 2025 13:54

alessandro-olivo merged commit 6a13f17 into mxcube:develop Jun 10, 2025
8 checks passed

alessandro-olivo mentioned this pull request Jun 16, 2025

Add protection against repeated clicks during manual centring mxcube/mxcubecore#1286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent negative clicks during manual centering and display a UI message #1649

Prevent negative clicks during manual centering and display a UI message #1649

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Prevent negative clicks during manual centering and display a UI message #1649

Prevent negative clicks during manual centering and display a UI message #1649

Uh oh!

Conversation

Uh oh!

The issue

The proposed change

Test the feature:

Thoughts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!