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

Add the ability to switch between tabs using keyboard controls #1363

Merged
merged 17 commits into from Apr 24, 2018

Conversation

@swoolcock
Copy link
Collaborator

@swoolcock swoolcock commented Feb 1, 2018

Adds a function to switch tabs in a TabControl based on an offset and optionally wrapping.
Also adds next/previous document platform-level actions to be used for switching tabs.

@peppy
Copy link
Member

@peppy peppy commented Feb 6, 2018

How is this used/tested?

@peppy peppy added this to the Candidate Issues milestone Feb 6, 2018
@peppy peppy added the area:UI label Feb 6, 2018
@swoolcock
Copy link
Collaborator Author

@swoolcock swoolcock commented Feb 6, 2018

I can make the osu! PR for it if you like, but it won't compile until the framework is updated. Essentially it lets you use platform actions to switch between chat tabs.

@peppy
Copy link
Member

@peppy peppy commented Feb 6, 2018

As in there has to be added tests to the framework to explore this functionality.

Also why are the actions not bound anywhere in the framework?

@swoolcock
Copy link
Collaborator Author

@swoolcock swoolcock commented Feb 6, 2018

The osu! change adds an action listener to the chat panel to switch channels, but I guess it be done by default (or optionally) on the framework side. Then all tab containers would get the functionality if they have a focused child. I will make a test case for it too.

@peppy
Copy link
Member

@peppy peppy commented Feb 6, 2018

I'd probably make it available for next/previous tab, at very least (toggleable works too).

@smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Mar 6, 2018

@swoolcock the referenced PR has been merged, feel free to apply any changes you previously wanted.

@peppy
Copy link
Member

@peppy peppy commented Apr 11, 2018

@swoolcock any chance of having the updated and testable in a testcase (preferrably automated)?

@swoolcock
Copy link
Collaborator Author

@swoolcock swoolcock commented Apr 12, 2018

I've been a combination of sick and busy the past couple of weeks, I will finish this up this weekend. I have a working visual test for it, too.

@swoolcock
Copy link
Collaborator Author

@swoolcock swoolcock commented Apr 16, 2018

Once this PR is merged I’ll force push the osu! PR as it will no longer have visual tests.

@@ -50,6 +50,8 @@ internal MacOSGameHost(string gameName, bool bindIPC = false)
new KeyBinding(new KeyCombination(new[] { InputKey.Super, InputKey.Delete }), new PlatformAction(PlatformActionType.LineEnd, PlatformActionMethod.Delete)),
new KeyBinding(new KeyCombination(new[] { InputKey.Super, InputKey.Shift, InputKey.Left }), new PlatformAction(PlatformActionType.LineStart, PlatformActionMethod.Select)),
new KeyBinding(new KeyCombination(new[] { InputKey.Super, InputKey.Shift, InputKey.Right }), new PlatformAction(PlatformActionType.LineEnd, PlatformActionMethod.Select)),
new KeyBinding(new KeyCombination(new[] { InputKey.Alt, InputKey.Super, InputKey.Left }), new PlatformAction(PlatformActionType.DocumentPrevious)),

This comment has been minimized.

@peppy

peppy Apr 24, 2018
Member

Are you sure these are the correct keys? I can't find any other mac app which uses these bindings.

This comment has been minimized.

@swoolcock

swoolcock Apr 24, 2018
Author Collaborator

Visual Studio Code, Google Chrome, Atom, Sublime Text.

@@ -237,6 +246,35 @@ protected virtual void SelectTab(TabItem<T> tab)
Current.Value = SelectedTab.Value;
}

public virtual void SwitchTab(int offset, bool wrap = true)

This comment has been minimized.

@peppy

peppy Apr 24, 2018
Member

Can we add xmldoc for this method. Might also worth adding tests for it directly, since it's public.

Also make a decision on whether offset should support values other than -1 and 1. Personally I'd rename offset to direction and throw exceptions if not -1 or 1, just to keep behaviour sane (right now for instance, calling with negative offsets will not wrap correctly for values below -1).

This comment has been minimized.

@swoolcock

swoolcock Apr 24, 2018
Author Collaborator

The tests for SwitchTab would essentially be copy/paste of the current visual tests, but with direct calls rather than firing PlatformActions. Is it worth it?

public virtual void SwitchTab(int direction, bool wrap = true)
{
if (Math.Abs(direction) != 1)
throw new ArgumentException("Switch direction must be 1 or -1.");

This comment has been minimized.

@peppy

peppy Apr 24, 2018
Member

$"{nameof(direction)} must be -1 or 1."

@peppy
peppy approved these changes Apr 24, 2018
Copy link
Member

@peppy peppy left a comment

👍

@peppy peppy removed the WIP label Apr 24, 2018
@peppy peppy modified the milestones: Candidate Issues, April 2018 Apr 24, 2018
@peppy peppy merged commit df4e92b into ppy:master Apr 24, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@peppy peppy changed the title Switchable TabControl Add the ability to switch between tabs using keyboard controls Apr 24, 2018
@swoolcock swoolcock deleted the swoolcock:switchable-tabs branch Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants