Change setModified to destructively overwrite target in OpenDisplayAction

[vincent-hve]

Middle or control click on a widget supporting Actions, changed the target field permanently in the associated OpenDisplayAction, causing this operation to be executed henceforth, even without the modifier keys, or middle mouse button.

This PR fixes that, such that the target is only active for as long as the modifier is set.

[georgweiss]

@vincent-hve, need to admit I do not quite understand the use case this PR fixes. Can you please clarify the workflow, and also attach a bob file?

[vincent-hve]

Its not a bob-file specific issue. Any OpenDisplayAction has a default target, normally this is TAB. Thus click on an ActionButton with an OpenDisplayAction (or other widget with an action) will open the bob file in a new tab.

However, additionally the OpenDisplayAction supports pressing middle mouse button (or left + ctrl) to open the bob file in a new window instead of a new tab.

This behavior was previously implemented by permanently changing the target field in the OpenDisplayAction, when processing the modifiers (e.g. ctrl or middle mouse instead of left mouse). Any subsequent clicks without modifiers on the ActionButton would still open the bob file in a new window, instead of a new tab as would be expected.

This unexpected behavior is fixed in this PR by adding an addition override target field, which is only used when the modifier is active, and keeping the original target field as-is.

[georgweiss]

@vincent-hve, thanks for the explanation.

However, on top of the observed issue I think there might be a bug or unintended behavior. Currently the code reads:

if (event.isShortcutDown() || middle_click) {
            target = Target.TAB;
        } else if (event.isShiftDown()) {
            target = Target.WINDOW;
        }

suggesting CTRL+left click would select TAB as the target. This I find odd as TAB is in a sense the default (as in the editor) and should not need a modifier key.

At the same time the display example (the "documentation") reads:

By holding the 'Control' key, the display will open in a new tab.
By holding the 'Shift' key, the display will open in a new workspace window.
By holding both 'Control' and 'Shift' key, the display will open in a standalone window.

The last case CTRL+SHIFT+left click (REPLACE) is obviously not supported in the current code.

My suggestion would be:

 public void setModifiers(final MouseEvent event) {
        boolean middle_click = event.isMiddleButtonDown();
        if (event.isShortcutDown() || middle_click) {
            target = Target.REPLACE;
        } else if (event.isShiftDown()) {
            target = Target.WINDOW;
        }
        else{
            target = Target.TAB;
        }
    }

This would solve the issue at hand while also supporting the REPLACE case. Granted, users that always use CTRL+left click and expect the target to be TAB will see a new behavior, which might be annoying.

@kasemir, @shroffk, what is your take on this?

[vincent-hve]

The target is configurable. So you can not assume that the default is TAB (Though it will be in 99% of the cases). For this reason, the original target value can not be overwritten by the modified handler. Maybe some actions need to be opened in a new window by default, or replaced:

You want to restore to configured functionality when the modifier is released.

As for the documentation:

• By holding the 'Control' key, the display will open in a new tab.
• By holding the 'Shift' key, the display will open in a new workspace window.
• By holding both 'Control' and 'Shift' key, the display will open in a standalone window.

I do not know what the difference is between workspace window and standalone window. Further more, replace does not seem to be an option here. I would suggest the following.

• By holding the 'Control'/'Meta' key, the display will open in a new tab.
• By holding the 'Shift' key, the display will open in a new window (and middle mouse)
• By holding both 'Control'/'Meta' and 'Shift' key, the display is replaced

This will give you all the options regardless of which default target has been chosen.

[georgweiss]

You're right, my bad. Depending on the wanted default target configured on the action, one needs to honor that when no modifier keys are used.

Thanks for adding the missing case (REPLACE).

I would maybe suggest:

- By holding the 'Control' key, the display will open in a new tab.
- By holding the 'Shift' key or press middle mouse button, the display will open in a new window.
- By holding both 'Control' and 'Shift' key, the display is replaced
On Mac, use Command key instead of Control.

This text is in https://github.com/ControlSystemStudio/phoebus/blob/master/app/display/model/src/main/resources/examples/controls_action_buttons.bob

[kasemir]

TAB is in a sense the default

No. Replacement should be the default, just like links on a web page will by default replace the current page with the linked one.
The documentation is correct:

By holding the 'Control' key, the display will open in a new tab.
By holding the 'Shift' key, the display will open in a new workspace window.
By holding both 'Control' and 'Shift' key, the display will open in a standalone window.

Without any key modifiers, the default should be the default target of actions, which is replacement.

[shroffk]

Ok,

So the documentation is the desired ( correct ) behaviour.

We should update the
private Target target = Target.TAB;
with
private Target target = Target.REPLACE;

The other documentation and code are correct so we don't need to change the key combination to behaviour mapping code

BUT

@vincent-hve has found what I think is a valid bug

The use of the modifiers should be a one time/temporary change in behaviour but as stated... once you use any of the modifiers the open display behaviour of the action button is permanently changed ( until the screen is closed )

I think the open display behaviour action should return to what is configured in the .bob file on all future clicks

@kasemir is there a use case/workflow I am missing?

[shroffk]

Based on @vincent-hve code the following would make the code consistent with the documentation and make the modifier based behaviour a one time thing

    /**
     * Default {@link Target} is new tab.
     */
    private Target target = Target.REPLACE;
    /**
     * The target selected based on keyboard/mouse modifiers pressed when invoking the action.
     * If set, this overrides the configured {@link #target}.
     */
    private Target modifierTarget = null;
    
  

public Target getTarget() {
    return modifierTarget != null ? modifierTarget : target;
}

    @Override
    public void setModifiers(final MouseEvent event) {
        boolean middle_click = event.isMiddleButtonDown();
        if (event.isShiftDown() && event.isControlDown()) {
            modifierTarget = Target.STANDALONE;
        } else if (event.isShortcutDown() || middle_click) {
            modifierTarget  = Target.TAB;
        } else if (event.isShiftDown()) {
            modifierTarget  = Target.WINDOW;
        } else {
            modifierTarget = null;
        }
    }

[kasemir]

We should update the
private Target target = Target.TAB;
with
private Target target = Target.REPLACE;

Yes, REPLACE should be the default.

Key combinations override the default, just like in a web browser. Slight confusion with Ctrl vs. Apple-Propeller-Command key.
The context menu offers another way to opening "The Page", "The Page (New Tab)" or "The Page (New Window)", just like web browsers offer "Open link in new tab" and the like in the context menu.

Regrettably, but also just as in a web browser, we allow changing the default when defining an action and updating the "target" for that action. That usually results in many open tabs/windows followed by complaints that everything is slowing down. I don't know what to do about that.

[shroffk]

@kasemir

The use of the modifiers should be a one time/temporary change in behaviour but as stated... once you use any of the modifiers the open display behaviour of the action button is permanently changed ( until the screen is closed )

Is it ok if we fix the following... Once a user uses the modifier to open a "replace" with "a new window"... I don't want all subsequent clicks on the actions to use new window (even when the modifiers are not pressed... this is what is happening now), the behaviour should go back to using the "replace"... this would also avoid the too many tabs/windows issues

@vincent-hve can you update your PR with the changes I suggested in #3720 (comment)
and test the behaviour

[kasemir]

@vincent-hve has found what I think is a valid bug
make the modifier based behaviour a one time thing
behaviour should go back to using the "replace"

Absolutely

[vincent-hve]

From what I can tell STANDALONE is deprecated. So, should we still support this?

    /**
         * Open standalone window
         *
         * @deprecated Was only used in RCP version.
         */
        @Deprecated
        STANDALONE(Messages.Target_Standalone);

I'm more inclined to make the CTRL+SHIFT operation REPLACE. This would allow you to force any of the 3 non-deprecated options. For now I have modified it in accordance to #3720 (comment) .

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[kasemir]

It was deprecated and not implemented, but I think that has changed, it's now implemented