Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: add ToolStripItem.SelectedChanged event #8548

Closed
v-elnovikova opened this issue Feb 1, 2023 · 6 comments
Closed

[API Proposal]: add ToolStripItem.SelectedChanged event #8548

v-elnovikova opened this issue Feb 1, 2023 · 6 comments
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented
Milestone

Comments

@v-elnovikova
Copy link
Contributor

v-elnovikova commented Feb 1, 2023

Background and motivation

This is a follow-up to #3572

.NET Core 3.1 introduced a breaking change in using Menus. MenuItem control was removed and ToolStripMenuItem is recommended as a replacement.

However, the new ToolStripMenuItem class is missing the equivalent to MenuItem's Select event, that fires when menu item is highlighted by mouse or keyboard.

This event is most commonly used to display hints/descriptions/feedback to user when they navigate through menu items.

Some workarounds are the following ToolStripMenuItem's events: MouseEnter, MouseLeave, MouseHover, etc. But this does not cover the selection of a ToolStripMenuItem via keyboard (e.g. arrow keys).

Adding a new SelectedChanged event to the ToolStripItem class could be a solution to the problem. The event fires every time the Selected property value changes, similar to AvailableChanged or EnabledChanged events.

API Proposal

namespace System.Windows.Forms
{
    public class ToolStripItem
    {
        ...
        public event EventHandler? SelectedChanged;
        protected virtual void OnSelectedChanged(EventArgs e);
    }
}

API Usage

Here the SelectedChanged event is used to show a hint whenever user selects menu item with mouse or keyboard:

ToolStripMenuItem menuItem = new();
Label hintLabel = new();

menuItem.SelectedChanged += (object sender, EventArgs e) => 
{
    hintLabel.Text = menuItem.Selected ? "Menu item selected!" : string.Empty;
};

Alternative Designs

No response

Risks

No response

@v-elnovikova v-elnovikova added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Feb 1, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged The team needs to look at this issue in the next triage label Feb 1, 2023
@stephentoub stephentoub transferred this issue from dotnet/runtime Feb 1, 2023
@ghost ghost added the 🚧 work in progress Work that is current in progress label Feb 1, 2023
@dreddy-work dreddy-work removed the untriaged The team needs to look at this issue in the next triage label Feb 8, 2023
@dreddy-work dreddy-work added untriaged The team needs to look at this issue in the next triage and removed untriaged The team needs to look at this issue in the next triage labels Mar 16, 2023
@v-elnovikova v-elnovikova added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed 🚧 work in progress Work that is current in progress labels May 30, 2023
@Tanya-Solyanik Tanya-Solyanik added this to the Future milestone Jul 18, 2023
@Tanya-Solyanik Tanya-Solyanik modified the milestones: Future, .NET 9.0 Jul 31, 2023
@bartonjs
Copy link
Member

bartonjs commented Aug 3, 2023

Video

Looks good as proposed

namespace System.Windows.Forms
{
    public partial class ToolStripItem
    {
        public event EventHandler? SelectedChanged;
        protected virtual void OnSelectedChanged(EventArgs e);
    }
}

@bartonjs bartonjs added api-approved (4) API was approved in API review, it can be implemented and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Aug 3, 2023
Epica3055 added a commit to Epica3055/winforms that referenced this issue Nov 16, 2023
Epica3055 added a commit to Epica3055/winforms that referenced this issue Nov 17, 2023
@Epica3055 Epica3055 self-assigned this Nov 20, 2023
Epica3055 added a commit to Epica3055/winforms that referenced this issue Dec 7, 2023
@Olina-Zhang
Copy link
Member

Tested the fix in the latest .Net 9.0 SDK build: 9.0.100-alpha.1.23618.4, when using keyboard to focus or leave toolStripMenuItem, the SelectedChanged event is invoked once, but when using mouse to focus, the SelectedChanged event is invoked twice. @Epica3055 could you please look at it if the behavior is expected?

SelectedChanged_Test.mp4

@Epica3055
Copy link
Member

@Tanya-Solyanik hi, I need to discuss this with you.

In framework, MenuItem have Select event. when the item's selection status turns from unselected to selected, the event will be fired. But when status turns from selected to unselected, the event won't be fired.

The question is what is expected behavior in .Net?

In the issue description, ToolStripItem.SelectedChanged , from semantic view, I think the event should be fired when the item's selection status turns from unselected to selected and from selected to unselected.

But that's not the case in Framework.

@Tanya-Solyanik
Copy link
Member

@Epica3055 - what you are saying about symmetric behavior makes sense, if we were implementing a completely new event, we would had raised it in both cases - Selected->Unselected and Unselected -> Selected. However, the purpose of this implementation is to simplify migration from .NET Framework, so we should implement the same behaviour as the .NET Framework has.

@elachlan
Copy link
Contributor

Closing issue as the PR and follow up have been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved (4) API was approved in API review, it can be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants