search

ControlSystemStudio / phoebus

A framework and set of tools to monitor and operate large scale control systems, such as the ones in the accelerator community.
http://phoebus.org/
Eclipse Public License 1.0
92 stars 89 forks source link

Boolean button behavior with confirmation #2423

Open georgweiss opened 2 years ago

georgweiss commented 2 years ago

If the Boolean Button widget is configured as "push" and to show a confirmation dialog (on set), the button does not return to default value after positive confirmation. As this is different compared to running without confirmation I assume this is a bug, but want to confirm before I tackle it.

kasemir commented 2 years ago

A brief glance at https://github.com/ControlSystemStudio/phoebus/blob/master/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/BoolButtonRepresentation.java suggests that we always call handlePress which then calls confirm, and in there we might check for confirmation or not, finally calling fireWrite or not.

So I don't see any difference in returning to the default value, it always seems wrong.

Should be like this:

1) User pushes button. The JavaFX button has already changed its state because of that, it's beyond our control. 2) We reset the button to the current PV state since we haven't written, yet. 3) Maybe we check for confirmation, that fails, so we do nothing. Or maybe we do write the "new" value 4) If we wrote the "new" value, and the PV accepted that, we'll receive an update from the PV and the button now changes to the received state.

Ideally, steps 2-4 happen so quickly that the user never notices any flicker. Looks like push, done; not push, pop back to previous state, then update to the pushed state. But if there's flicker, so be it. We certainly want to avoid this case:

georgweiss commented 2 years ago

Thanks, but not sure this helps me. What is the intension using the boolean button in "push" mode? From the example OPIs it seems that 1 is written to the specified bit, then immediately afterwards 0 is written. Or is the 0 written because button returns to "default" state?

kasemir commented 2 years ago

"Push" writes 1 on mouse-down, then 0 on mouse-release. The intended usage is for example moving a motor as long as the button is pushed down, then stop when released. So typically held down for a little while.

"Toggle" changes between 0 and 1 on each click. For example used to turn a power supply on/off.

kasemir commented 2 years ago

Since "push" tends to be used in a way where the operator pushes down, checks for something to happen, keeps mouse held down, then releases when "done", the operator is part of the feedback loop to verify that we're actually writing. Adding the proper return-button-to-default, write, then update-button-on-monitor may be overkill here.

But for "toggle", thinking that a PS was turned on when really nothing happened would be unfortunate.

georgweiss commented 2 years ago

OK thanks. So I guess using push together with confirmation dialog in practice turns the push mode to a toggle mode.

kasemir commented 2 years ago

It can't really work with "push". You push down, and should keep it pressed down for a while. But you can't, because you need to handle the confirmation dialog...

georgweiss commented 2 years ago

So maybe we should disable use of confirmation dialog with push?

kasemir commented 2 years ago

Ideally, yes, but it's a little tricky. The editor fundamentally just lists widgets and their properties. It doesn't "understand" what they do. You can configure both the background and the foreground color of a text to be black, the editor won't explain to you why you no longer see anything. Likewise, the editor doesn't know about the Bool Button with its toggle mode vs. confirmation behavior. So this would have to be in the bool button model or maybe even representation: If it notices "push" with "confirm", it changes one or both of them to remain compatible, and the editor just shows the current settings.

georgweiss commented 2 years ago

Well, if we can agree that "push" shall not support confirmation dialog then I can give it a try... Isn't this a matter of acting on property changes? Maybe I'm oversimplifying...

kasemir commented 2 years ago

"push" shall not support confirmation dialog

Yes, I'd say there's no practical way to keep pushing down for a while and handle confirmation at the same time.

georgweiss commented 2 years ago

So I looked into this and here is a proposal:

If user selects Push/Push inverted, the confirmation dialog setting will automatically set to "no". If user then tries to select confirmation dialog, the value stays unchanged. For this to work I need to change in EnumWidgetPropertyBinding#L42:

if (updating ||  jfx_node.isFocused())

to

if (updating)

Question is also whether to launch an alert with suitable message if user wants to configure an unsupported combination of mode and confirmation dialog.

kasemir commented 2 years ago

Question is also whether to launch an alert with suitable message if user wants to configure an unsupported combination of mode and confirmation dialog.

Not sure how to do that in a clean way.

The logic for de-selecting the confirmation when mode is "push" could be in the bool button widget model. If making that work requires a generic tweak to the editor (EnumWidgetPropertyBinding), fine.

What I'd like to avoid is something like if (widget_model instanceof BoolButtonWidget) .. in the editor, because the editor will ideally have no idea about any specific widget. It simply lists all the properties of all the registered widgets knowing anything else about them. The bool button widget model can't have GUI code for opening a dialog. The only code that has access to the GUI and knows about details of the bool button is the BoolButtonRepresentation. In its createJFXNode, there is already a section if ... isEditMode(). So maybe you put the logic for handling acceptable property value combinations and the code for opening a "You can't do that" dialog into the BoolButtonRepresentation?

georgweiss commented 2 years ago

Ideally I'd like to disable the confirmation dialog drop-down if user selects anything but toggle. Is there an API for that? But even so, supporting existing display files using the unsupported combination complicates matters.

I was planning to put everything in the BoolButtonRepresentation, yes. Maybe better to simply launch a dialog when user selects an unsupported combination, and then revert back to previous value for either mode or confirmation dialog property.

kasemir commented 2 years ago

No, there's nothing to enable/disable/add/hide properties in the editor.

Good to put everything into the BoolButtonRepresentation. It it's only activated if .. isEditMode, then existing displays would simply execute as before, certainly not opening information dialogs meant for edit-time at runtime.

That leaves what to do at edit time with older displays. Would they open a dialog for each misconfigured bool button? Or only when changing a bool button to a conflicting setting?

georgweiss commented 2 years ago

Since a display could contain multiple such boolean buttons, it would be quite annoying to have to dismiss all dialogs. Moreover, this issue was discovered in a display from a user that is pretty new to CS Studio, so let's assume it's a beginner's mistake.

In short: existing displays would show dialog only of these properties are changed.