2129 pilx multiselection , click / double-click approach

[EdgarGF93]

Now, single click plots multi-selection by default.
Double-click plots only one plot (substitutes all the time)
Thoughts about this approach? Single click should be one-plot and double-click multiselection?
Do not use double click at all?

[kif]

Can you ping Bjorn to make him validate the feature ? Apparently, he has no GH account.

[loichuder]

I tested it and it doesn't feel right to me. The interaction is not really discoverable and not adapted to the usecase in my opinion. I have a few suggestions to improve it:

First, I would have kept the single-click interaction for displaying one curve at once to keep things simple.

Then, If we think about the usecase: to take the decision that we want to keep a particular pattern displayed, we must first see it. So I think the interaction could be: first a click on the map to display the curve and then a click on the curve to select it to keep it displayed (perhaps via the opening of a contextual menu) ? For this, the silx capability of selecting curves in the curve plot should be re-enabled (it is disabled for now).

Finally, it would helpful to also have a button in the toolbar to clear the displayed curves.

[EdgarGF93]

context menu approach: by right-clicking on the map, we get the options of adding the current pixel into the plot or using it as a background. If we select the background into the current background pixel, it's removed.

[loichuder]

My thought was to have the context menu on the curve plot but your solution does not look too shabby either! I find it much better 🙂 ! Great idea to have integrated the background in the context menu as well.

Unfortunately, I could not test it locally (the context menu entries do nothing when clicked on and I have an error AttributeError: 'MapPlotWidget' object has no attribute 'plotDoubleClicked'). Do you have some commits left to push ?

I notably wanted to test some edge cases:

• What happens if you single click on a curve that is already displayed ?
• What happens if you single click on the background curve location ?
• Can you add the same curve multiple times on the plot ?

Anyway, I reckon you can carry on with this implementation and ask for a review once you are happy with your solution.

[EdgarGF93]

sorry, I made a bloody mess with the commits, could you try now?

[kif]

LGTM ... I just got an exception for:

ERROR:silx.gui.plot.tools.PositionInfo:Error while converting coordinates (81.000000, 57.000000)with converter 'Data'
ERROR:silx.gui.plot.tools.PositionInfo:Traceback (most recent call last):
  File "/users/kieffer/.venv/py312/lib/python3.12/site-packages/silx/gui/plot/tools/PositionInfo.py", line 299, in _updateStatusBar
    value = func(xData, yData)
            ^^^^^^^^^^^^^^^^^^
  File "/users/kieffer/.venv/py312/lib/python3.12/site-packages/pyFAI/gui/pilx/widgets/MapPlotWidget.py", line 94, in _dataConverter
    index = self.getScatterIndex(x, y)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/users/kieffer/.venv/py312/lib/python3.12/site-packages/pyFAI/gui/pilx/widgets/MapPlotWidget.py", line 168, in getScatterIndex
    pixel_x, pixel_y = self.dataToPixel(x_data, y_data)
    ^^^^^^^^^^^^^^^^
TypeError: cannot unpack non-iterable NoneType object

This pattern exists at several places in the code and is probably not related to your PR it is never nice to have such exception raising ...

[kif]

I made a new issue on this ...
#2142

[kif]

LGTM

[loichuder]

Sorry for being bothersome but there is a still some UX adjustements to be made in my opinion. From what I got from the beamline, the main use case is to keep one pattern displayed and to compare it with several ones from the map.

With the current PR, when I add a curve to the graph via Add curve to the graph, it disappears when I click on another map location. So the only way to achieve the usecase mentionned above is to right-click and select the context menu entry on each and every curve I want to compare. Which I find not great in terms of UX.

I think the context menu entry should instead be Keep curve displayed. The curve then stays displayed unless the user explicitly asks to clear it.

You will have to tackle this in another PR since this is merged now.

[EdgarGF93]

Yes, the issue is still open and I'll create a new PR