refactor: (core/TabControl) TabControl event interface change #399
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the TabControl and TabItem event handling system by replacing a custom event system with standard WPF routed events, improving alignment with WPF best practices and simplifying the API surface.
Key Changes:
- Introduced WPF routed events (
TabItemClosingEventandAddTabButtonClickEvent) inTabControlHelperwith standard Add/Remove handler methods, replacing the previousTabControlHelperEventsandTabItemHelperEventsclasses - Refactored
TabViewTabCloseRequestedEventArgsto inherit fromRoutedEventArgs, enabling proper routed event propagation - Updated gallery samples to demonstrate the new event handling approach using XAML event attachment syntax
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| source/iNKORE.UI.WPF.Modern/Controls/TabItemExtension.cs | Removed deprecated extension methods for registering tab closing events |
| source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabItemHelper.cs | Removed TabItemHelperEvents class and updated event raising to use routed events |
| source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabControlHelper.cs | Added routed event definitions with Add/Remove handler methods; refactored TabViewTabCloseRequestedEventArgs to inherit from RoutedEventArgs; removed legacy TabControlHelperEvents class |
| source/iNKORE.UI.WPF.Modern.Gallery/Pages/Controls/Windows/TabViewPage.xaml.cs | Updated sample to use new routed event handler instead of RegisterTabClosingEvent extension method; updated example code strings |
| source/iNKORE.UI.WPF.Modern.Gallery/Pages/Controls/Windows/TabViewPage.xaml | Added XAML event attachment syntax for TabItemClosing event on TabControl |
| samples/WpfApp1/WpfApp1.csproj | Fixed project reference path casing for consistency |
| samples/WpfApp1/MainWindow.xaml | Updated sample UI to use SimpleStackPanel and Expander controls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static readonly RoutedEvent TabItemClosingEvent = EventManager.RegisterRoutedEvent( | ||
| "TabItemClosing", | ||
| RoutingStrategy.Direct, | ||
| typeof(EventHandler<TabViewTabCloseRequestedEventArgs>), | ||
| typeof(TabControlHelper)); | ||
|
|
||
| public static TabControlHelperEvents GetTabControlHelperEvents(TabControl element) | ||
| public static void AddTabItemClosingHandler(TabControl tabControl, EventHandler<TabViewTabCloseRequestedEventArgs> handler) | ||
| { | ||
| tabControl.AddHandler(TabItemClosingEvent, handler); | ||
| } | ||
| public static void RemoveTabItemClosingHandler(TabControl tabControl, EventHandler<TabViewTabCloseRequestedEventArgs> handler) | ||
| { | ||
| return (TabControlHelperEvents)element.GetValue(TabControlHelperEventsProperty); | ||
| tabControl.RemoveHandler(TabItemClosingEvent, handler); | ||
| } |
There was a problem hiding this comment.
The routed event TabItemClosingEvent and its associated handler methods (AddTabItemClosingHandler, RemoveTabItemClosingHandler) lack XML documentation comments. Consider adding documentation to explain their purpose, parameters, and usage, similar to other public API members in this class.
| public static readonly RoutedEvent AddTabButtonClickEvent = EventManager.RegisterRoutedEvent( | ||
| "AddTabButtonClick", | ||
| RoutingStrategy.Direct, | ||
| typeof(RoutedEventHandler), | ||
| typeof(TabControlHelper)); | ||
|
|
||
| public static void AddAddTabButtonClickHandler(TabControl tabControl, RoutedEventHandler handler) | ||
| { | ||
| tabControl.AddHandler(AddTabButtonClickEvent, handler); | ||
| } | ||
|
|
||
| public static void RemoveAddTabButtonClickHandler(TabControl tabControl, RoutedEventHandler handler) | ||
| { | ||
| tabControl.RemoveHandler(AddTabButtonClickEvent, handler); | ||
| } |
There was a problem hiding this comment.
The routed event AddTabButtonClickEvent and its associated handler methods (AddAddTabButtonClickHandler, RemoveAddTabButtonClickHandler) lack XML documentation comments. Consider adding documentation to explain their purpose, parameters, and usage, similar to other public API members in this class.
| /// Provides data for a tab close event. | ||
| /// </summary> | ||
| public sealed class TabViewTabCloseRequestedEventArgs | ||
| public class TabViewTabCloseRequestedEventArgs : RoutedEventArgs |
There was a problem hiding this comment.
[nitpick] The sealed modifier was removed from the TabViewTabCloseRequestedEventArgs class. This is a potential breaking change that allows inheritance. If this was intentional to support extensibility, it's fine, but if the class was originally sealed to prevent subclassing, consider whether this change is appropriate. Removing sealed is generally safe but changes the API contract.
| public class TabViewTabCloseRequestedEventArgs : RoutedEventArgs | |
| public sealed class TabViewTabCloseRequestedEventArgs : RoutedEventArgs |
| internal TabViewTabCloseRequestedEventArgs(RoutedEvent routedEvent, object item, TabItem tab) | ||
| : base(routedEvent) | ||
| { | ||
| Item = item; | ||
| Tab = tab; | ||
| } |
There was a problem hiding this comment.
[nitpick] The constructor of TabViewTabCloseRequestedEventArgs now requires a RoutedEvent parameter. This is a breaking change for any code that was directly constructing this class. Consider whether this constructor should remain internal (which it is) or if there should be a public constructor overload that doesn't require the RoutedEvent parameter for backward compatibility scenarios.
| if (eventArgs.Cancel) | ||
| var eargs = new TabViewTabCloseRequestedEventArgs(TabControlHelper.TabItemClosingEvent, TabItem.Content, TabItem); | ||
| TabControl.RaiseEvent(eargs); | ||
|
|
There was a problem hiding this comment.
[nitpick] Line 213 has trailing whitespace after TabControl.RaiseEvent(eargs);. Consider removing it to maintain code cleanliness.
| if ((e.Tab.Header as string) != NiceTry) | ||
| e.Tab.Header = NiceTry; | ||
| else e.Tab.Header = "You can't close me!"; |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if ((e.Tab.Header as string) != NiceTry) | |
| e.Tab.Header = NiceTry; | |
| else e.Tab.Header = "You can't close me!"; | |
| e.Tab.Header = ((e.Tab.Header as string) != NiceTry) ? NiceTry : "You can't close me!"; |
This pull request refactors how tab closing events are handled in the TabControl and TabItem components, replacing the previous custom event system with standard WPF routed events. This change simplifies the event wiring, improves maintainability, and aligns with WPF best practices. Additionally, related sample code and documentation are updated to reflect the new approach.
TabControl and TabItem event handling refactor:
TabItemClosingEventandAddTabButtonClickEvent) inTabControlHelper, replacing the previousTabControlHelperEventsandTabItemHelperEventsclasses for tab closing and add button click events. Added methods to attach/detach handlers using WPF's event system. (source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabControlHelper.cs, [1] [2] [3] [4];source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabItemHelper.cs, [5] [6] [7]TabViewTabCloseRequestedEventArgsto inherit fromRoutedEventArgs, enabling routed event propagation and standard handling. (source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabControlHelper.cs, [1] [2]TabItemExtension.csand related code inTabControlHelperEventsandTabItemHelperEvents. (source/iNKORE.UI.WPF.Modern/Controls/TabItemExtension.cs, [1];source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabControlHelper.cs, [2];source/iNKORE.UI.WPF.Modern/Controls/Helpers/TabItemHelper.cs, [3] [4]Sample and documentation updates:
TabViewPage.xamlandTabViewPage.xaml.cs) to use the new routed event approach for tab closing, replacing the previous registration method and event handler implementation. (source/iNKORE.UI.WPF.Modern.Gallery/Pages/Controls/Windows/TabViewPage.xaml, [1];source/iNKORE.UI.WPF.Modern.Gallery/Pages/Controls/Windows/TabViewPage.xaml.cs, [2] [3] [4]WpfApp1.csprojfor consistency. (samples/WpfApp1/WpfApp1.csproj, samples/WpfApp1/WpfApp1.csprojL16-R16)MainWindow.xamlto useSimpleStackPanelandExpandercontrols, removing redundant test buttons. (samples/WpfApp1/MainWindow.xaml, samples/WpfApp1/MainWindow.xamlL14-R29)