Fixes #4050 - `Select`->`Activate`

[tig]

Fixes

• Fixes #4050

Proposed Changes/Todos

• [x] Command.Select->Activate
• [x] Rename Selecting event to Activating, OnSelecting to OnActivating, and RaiseSelecting to RaiseActivating in the View class and all derived classes (e.g., Menuv2, MenuItemv2, CheckBox, FlagSelector).
• [x] Update related code:
  • [x] Modify SetupCommands in View to use Command.Activate.
  • [x] Update Shortcut.DispatchCommand and other command handlers to reference Activating.
  • [x] Ensure all event handlers and documentation reflect the new terminology.
• [x] Update Documentation
  • [x] Clarify Activating vs. Accepting:
  • [x] Update view-specific docs:
• [ ] Go through built-in Views and ensure Selected / Activated terminology is correct (e.g. TreeView, TableView, ...)

For another PR

I was going to do this here, but I'll do it in another PR:

• [ ] Introduce Targeted Propagation Model with PropagateActivating
  • [ ] Document propagation: Explain the targeted model with PropagateActivating, providing examples for MenuBarv2 and potential Toplevel use cases.

[tig]

I'm not convinced we don't still need a Command.Select in ADDITION to Command.Accept.

Code like this in TableView...

        // BUGBUG: OnCellActivated is misnamed, it should be OnCellAccepted? Or is it OnCellSelected?
        // BUGBUG: Does this mean we still need Command.Select?
        AddCommand (Command.Accept, () => OnCellActivated (new (Table, SelectedColumn, SelectedRow)));

And TreeView:

    /// <summary>
    ///     <para>Triggers the <see cref="ObjectActivated"/> event with the <see cref="SelectedObject"/>.</para>
    ///     <para>This method also ensures that the selected object is visible.</para>
    /// </summary>
    /// <returns><see langword="true"/> if <see cref="ObjectActivated"/> was fired.</returns>
    public bool? ActivateSelectedObjectIfAny (ICommandContext commandContext)
    {
        // By default, Command.Accept calls OnAccept, so we need to call it here to ensure that the event is fired.
        if (RaiseAccepting (commandContext) == true)
        {
            return true;
        }

        T o = SelectedObject;

        if (o is { })
        {
            // TODO: Should this be cancelable?
            ObjectActivatedEventArgs<T> e = new (this, o);
            OnObjectActivated (e);

            return true;
        }

        return false;
    }

???

[BDisp]

I think it make sense to have both. Command.Select doesn't mean it was accepted. So, adding Command.Accept will mean it was confirmed and will perform some action, like closing or opening a view, etc.... Command.Select also may perform some action but not definitely. Both must raise the respective events.

[tznind]

[we] still need a Command.Select in ADDITION to Command.Accept

yes table and tree view both distinguish between selecting (focusing) a cell/object and activating (opening) it.

[BDisp]

Since you're dealing with Select/Activate it's normal a Button handles Accept when a Shortcuts in a ScrollBar are clicked?

[BDisp]

There is one situation in Terminal.Gui which make me crazy. A simple Accepting event which will allow the developer to Cancel the default behavior (but has the Handled property) will repeat the event 3 times by mouse if the developer doesn't Cancel it. UICatalog has a bunch of Button's that if they are activated by mouse will repeat the event 3 times, no matter the user press Key.Enter or Key.Esc. Despite by adding Handled = true fix this, there is no necessity of this extra code for a normal Accepting event. We can't handle mouse event as key event because only the only the view where the mouse is will be executed and doesn't not propagate to other views and with key event it can be propagate to other views that may want to handle it. I agree the leverage of keybindings, commands and context to achieve this behavior but we need to handle the mouse with much more careful.

[tig]

I'm not entirely sure what you mean. But I am working through this here.

Can you create a simple sample or unit test that illustrates?

[BDisp]

It's only enough to comment the line e.Handled = true; in the Generic scenario. There are many scenarios where this isn't implemented and I think it's an unnecessary extra code. The default behavior when a Button is clicked by mouse it only raise once. If you press Key.Enter you'll see that this issue doesn't happens even the commented code. It's only happens with mouse. I still don't agree with the need of adding e.Handled = true on this eevnt. Probably will make much more sense in a Selecting event which can be canceled and if it isn't canceled an event Accepted not cancelable will being raise.

        button.Accepting += (s, e) =>
                            {
                                // When Accepting is handled, set e.Handled to true to prevent further processing.
                                //e.Handled = true;
                                MessageBox.ErrorQuery ("Error", "You pressed the button!", "_Ok");
                            };

[BDisp]

I really can't repro this with the FakeDriver but I already found that all drivers really send a clicked event with the Button position when no mouse button pressed is performed. Seems that all drivers are treat mouse movement as clicks more 2 times. @tznind do you know what is causing this?

[tznind]

Hmn there is various low level logging that can be turned on. I would guess it is more in Application layer though if it affects v2 drivers also.

Especially since Handled Fixes, it implies its same event going round and round

[BDisp]

That what I was think but the wrong click event came from the drivers. Do Application is injecting the click event into the driver?

[BDisp]

Sorry, the driver has still the previous position when the Button was pressed. It isn't a resent event. The mouse must return true at the end of the method, otherwise it will process as a hotkey which isn't the same as a Key event.

[BDisp]

The CommandContext struct could probably be converted to a class, thus allowing it to be more volatile and manageable.

[tig]

This is ready, I think.

I'm NOT fixing Menuv2 in this. That will be in another PR.

[BDisp]

Do you know why Trace item it's always selected at start instead of the activated Debug item?

[tig]

Do you know why Trace item it's always selected at start instead of the activated Debug item?

Focus etc... is broken in MenuV2.

[tig]

Do you know why Trace item it's always selected at start instead of the activated Debug item?

Focus etc... is broken in MenuV2.

In my refactor of OptionSelector (see SelectorBase etc...) I removed the code that was trying to make them look right in MenuV2 because it was not correct. I'm now working on bringing that back in a more correct way.

[tig]

I've converted this back to draft. I have discovered/remembered I have tons more to do here to make this really work.

That said, I finally got OptionSelector and FlagSelector looking and working mostly correctly when added as a Shortcut.CommandView, making Menuv2 much better. There are still some issues (related to the how Command.Accepting/Activating propogate).

[BDisp]

@tig can a MenuBarv2 having a MenuItemv2 with an Action or is mandatory only using MenuBarItemv2?

Edit: Or maybe more appropriate, can a MenuBarv2 having a MenuBarItemv2 with an Action?

[tig]

IIRC my intent was both What you describe should work. But I doubt I ever tested MenuBarv2 containing a MenuItemv2.

The end user scenario is a MenuBar item that acts just like a StatusBar item.

[BDisp]

Works like a charm. Doesn't make sense to set Action in a MenuBarItemv2 because it will handle popover menus, although it's possible but the Action will be ignored. With MenuItemv2 works perfectly. Thanks.

[BDisp]

@BDisp I moved your comment to the Issue where it belongs: https://github.com/gui-cs/Terminal.Gui/issues/4170

[tig]

@copilot can you fix these merge conflicts?

[tig]

Examples/UICatalog/Scenarios/SendKeys.cs Examples/UICatalog/Scenarios/Shortcuts.cs Terminal.Gui/App/CWP/CWPPropertyHelper.cs Terminal.Gui/ViewBase/View.Drawing.Attribute.cs Terminal.Gui/ViewBase/View.Drawing.Scheme.cs Terminal.Gui/ViewBase/View.Navigation.cs Terminal.Gui/Views/CharMap/CharMap.cs Tests/UnitTests/Views/RadioGroupTests.cs Tests/UnitTestsParallelizable/View/Navigation/AdvanceFocusTests.cs

[codecov[bot]]

Codecov Report

:x: Patch coverage is 73.12500% with 86 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 73.39%. Comparing base (fc818b0) to head (1f6825b).

Files with missing lines	Patch %	Lines
Terminal.Gui/Views/FileDialogs/FileDialog.cs	23.80%	15 Missing and 1 partial :warning:
Terminal.Gui/ViewBase/View.Mouse.cs	88.37%	8 Missing and 2 partials :warning:
Terminal.Gui/Views/ListView.cs	58.82%	3 Missing and 4 partials :warning:
Terminal.Gui/Views/Shortcut.cs	83.72%	5 Missing and 2 partials :warning:
Terminal.Gui/Views/ScrollBar/ScrollBar.cs	28.57%	4 Missing and 1 partial :warning:
Terminal.Gui/ViewBase/View.Command.cs	71.42%	0 Missing and 4 partials :warning:
Terminal.Gui/Views/Color/ColorPicker.16.cs	20.00%	0 Missing and 4 partials :warning:
Terminal.Gui/Views/Label.cs	69.23%	2 Missing and 2 partials :warning:
Terminal.Gui/Views/Selectors/OptionSelector.cs	20.00%	1 Missing and 3 partials :warning:
....Gui/Views/TableView/CheckBoxTableSourceWrapper.cs	33.33%	4 Missing :warning:
... and 13 more

Additional details and impacted files

@@              Coverage Diff               @@
##           v2_develop    #4126      +/-   ##
==============================================
- Coverage       74.41%   73.39%   -1.02%     
==============================================
  Files             388      388              
  Lines           46568    46586      +18     
  Branches         6548     6540       -8     
==============================================
- Hits            34653    34192     -461     
- Misses          10053    10519     +466     
- Partials         1862     1875      +13     

Files with missing lines	Coverage Δ
Terminal.Gui/ViewBase/View.Keyboard.cs	84.23% <100.00%> (-1.19%)	:arrow_down:
Terminal.Gui/Views/Bar.cs	84.15% <100.00%> (+0.17%)	:arrow_up:
Terminal.Gui/Views/ComboBox.cs	80.00% <100.00%> (ø)
Terminal.Gui/Views/Menu/MenuBarv2.cs	76.70% <100.00%> (-5.23%)	:arrow_down:
Terminal.Gui/Views/Menu/PopoverMenu.cs	75.83% <ø> (-13.10%)	:arrow_down:
Terminal.Gui/Views/Menuv1/MenuItem.cs	48.18% <100.00%> (ø)
Terminal.Gui/Views/MessageBox.cs	63.71% <100.00%> (ø)
Terminal.Gui/Views/NumericUpDown.cs	85.05% <100.00%> (ø)
Terminal.Gui/Views/ScrollBar/ScrollSlider.cs	71.00% <100.00%> (ø)
Terminal.Gui/Views/Slider/Slider.cs	56.19% <100.00%> (ø)
... and 31 more

... and 38 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fc818b0...1f6825b. Read the comment docs.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.