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
Add event handler for user change of TabControl
tab selection
#6218
Conversation
@@ -378,10 +403,13 @@ private void selectTab(TabItem<T> tab) | |||
/// </summary> | |||
/// <param name="direction">Pass 1 to move to the next tab, or -1 to move to the previous tab.</param> | |||
/// <param name="wrap">If <c>true</c>, moving past the start or the end of the tab list will wrap to the opposite end.</param> | |||
public virtual void SwitchTab(int direction, bool wrap = true) | |||
/// <returns>Whether tab selection has changed as a result of this call.</returns> | |||
public virtual bool SwitchTab(int direction, bool wrap = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this method doesn't signal a user selection but SelectItem
does? How does that make sense naming / exposure wise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not realise this method is public, thought it was protected. We can signal the event from here and define a private version that's used by the "switch tab on remove" case.
The reason why I don't want the sound to play on the "switch tab on remove" scenario is because there would already be a sound played when the user removes the tab (e.g. a click sound on the "close" button).
@@ -349,16 +352,37 @@ private void updateDropdown(TabItem<T> tab, bool isVisible) | |||
} | |||
|
|||
/// <summary> | |||
/// Selects a <see cref="TabItem{T}"/>. | |||
/// Selects a <see cref="TabItem{T}"/> and signals an event that the user selected the given tab value via <see cref="OnUserTabSelectionChanged"/>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno about all these public events triggering the OnUserTabSelectionChanged
method, but let's see how it works.
I've made further simplifications to convoluted logic. Requesting a second review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine
Add event handler for user change of `TabControl` tab selection
Add event handler for user change of `TabControl` tab selection
The abstraction may look a little bit too much, but I've been trying to find a middle-ground between code readability and simplicity of structure.
The previously existing
SelectTab
method is supposed to do all that's necessary for the tab selection to be changed, without necessarily being related to user action, so I've renamed it toUpdateTabSelection
.I've also introduced two methods,
SelectTab
andSelectItem
, the former is for internal use byTabItem
click event to update tab selection and trigger user event, meanwhile the latter is for external use by calling it with the target item to update tab selection and trigger user event as well (e.g. osu! calling the method when changing editor screen mode, so that an auxiliary sample is played).I didn't want
SelectTab
to exist in the first place, but it's necessary so that theTabItem
click event doesn't have to callSelectItem
and perform an unnecessary lookup for an already known tab item, so that's why it's there.