-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
WIP: Keyboard only operation #1074
Conversation
Just looking at Travis it seems like failures are formatting, test failures and linting - I'd recommend the git hook we use https://github.com/fyne-io/fyne/wiki/GitHooks |
@@ -187,7 +187,12 @@ func loadWindowGroup() fyne.Widget { | |||
}), | |||
widget.NewButton("Fixed size window", func() { | |||
w := fyne.CurrentApp().NewWindow("Fixed") | |||
w.SetContent(fyne.NewContainerWithLayout(layout.NewCenterLayout(), widget.NewLabel("Hello World!"))) | |||
w.SetContent(fyne.NewContainerWithLayout( |
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'm not sure what this is demoing?
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.
It was introduced to test keyboard operation in a second window. There was no extra windows with any focusable widgets, so I had to introduce some buttons to test it. Of cause it failed the first time I tested it, but should be working ok now.
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.
All windows should behave the same though?
internal/app/focus.go
Outdated
@@ -13,7 +13,12 @@ type FocusManager struct { | |||
func (f *FocusManager) nextInChain(current fyne.Focusable) fyne.Focusable { | |||
var first, next fyne.Focusable | |||
found := current == nil // if we have no starting point then pretend we matched already | |||
driver.WalkVisibleObjectTree(f.canvas.Content(), func(obj fyne.CanvasObject, _ fyne.Position, _ fyne.Position, _ fyne.Size) bool { | |||
//OBS driver.WalkVisibleObjectTree(f.cavas.Content(), func(obj fyne.CanvasObject, _ fyne.Position, _ fyne.Position, _ fyne.Size) bool { |
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.
OBS? try not to commit commented out code
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.
This may relate to bug #814
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 use //OBS to easily do a global search for comments that should be removed before commit. This fell through the cracks.
As for but #814, I belive the added lines of code will be a fix for the bug. I am not able to reproduce it.
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 agree. In the future it's good to have a bug fix in separate commit - possibly even PR - than a big feature addition
i.Parent.activateChild(i) | ||
for j := 0; j < len(i.Parent.Items); j++ { |
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 don't understand this loop...
i.Parent.Items[i.Parent.index]
is i
so the item that has just been activated is being defocussed (or "upped").
Possibly this is because I don't understand what index
is
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 have changed the name "index" to "currentItem", as it indicates the menu item selected. There is unfortunately a mix of "focused". The focused item receiveing the keyboard data is the MenuBar, while the indicated (focus-colored) item is one of the menu items in the list. Perhaps each menu item should recieve keyboard input?
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.
THat might be cleaner, would probably help with code encapsulation, that said left/right may still make sense at the top level.
internal/driver/glfw/menu_bar.go
Outdated
Items []fyne.CanvasObject | ||
|
||
Items []fyne.CanvasObject | ||
index int |
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.
index is quite generic, not clear what this means
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.
index is the currently focused item on the menu bar. Using arrow keys, the focus is moved and index is updated. This is also used on toolbars and tabs.
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 maybe activeIndex
or currentMenu
naming then?
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.
Just changed it to currentItem
internal/driver/glfw/menu_bar.go
Outdated
} | ||
|
||
// HandleKey will process keys pressed when menu is open | ||
func (b *MenuBar) HandleKey(key fyne.KeyName) { |
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.
This should not be an exported API
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.
Ok
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.
By way of explanation here (sorry it was lacking before) we don't add a new exported API unless there is a solid use-case for developers.
widget/tabcontainer.go
Outdated
@@ -20,6 +20,8 @@ type TabContainer struct { | |||
OnChanged func(tab *TabItem) | |||
current int | |||
tabLocation TabLocation | |||
focused bool | |||
renderer tabContainerRenderer |
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.
This is a no-no - a widget must not retain a reference to it's current renderer
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.
This is one of the difficult, deep issues. I could not find any other way to do it.
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.
From the code that uses it it's hard to see the intent, was it to change buttons or highlight that one had been pressed? if it's the latter then maybe tabButton is the place for that code? otherwise the keyboard handling code can change selected item that the render then updates to reflect.
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.
Actualy, on the toolbar, this functionality is not needed and has been removed. But on the Toolbar, there is a similar use of the renderer.
The problem is that the toolbar must update the state of the boolbar buttons, as it receives key-presses. But it has no reference to its buttons. To avoid the renderer reference, I have instead added a buttons array in the toolbar.
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 guess this is a little better than holding the renderer, but given that a renderer's lifecycle can be much shorter than a widget it still seems a little risky - you are capturing an internal detail of the renderer could will become invalid if the renderer is destroyed.
Renderers are only guaranteed to remain as long as the widget is visible - if it is hidden for any reason the renderer could be detached.
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.
Oops. Is there a way to force detachement of all renderers, so this can be tested?
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.
Currently we do not have that test capability, but it’s a good suggestion.
widget/tabcontainer.go
Outdated
} | ||
|
||
func (b *TabContainer) KeyUp(key *fyne.KeyEvent) { | ||
if b.renderer.tabBar==nil { |
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.
Sorry, no - a widget must not manually manipulate the state of it's renderer.
Widgets manage state - renderers draw it.
Surely calling SelectTab
or similar helper is the right thing to do?
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.
This has been fixed now
widget/tabcontainer.go
Outdated
@@ -463,15 +531,6 @@ func (b *tabButton) Tapped(e *fyne.PointEvent) { | |||
b.OnTap() | |||
} | |||
|
|||
func (b *tabButton) setText(text string) { |
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.
Please don't rearrange methods unless they are in the wrong order.
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 have tried to change as little as possible on the existing code, but is there a description of what is correct order?
Anyway, I have moved setText() back to where if was.
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 don't think we have it written up the wiki yet, the information is in issue #827
widget/toolbar.go
Outdated
button := NewButtonWithIcon("", t.Icon, t.OnActivated) | ||
button.HideShadow = true | ||
|
||
//button := NewButtonWithIcon("", t.Icon, t.OnActivated) |
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.
Please don't commit commented out code - we have version control to track changes.
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 know. I just missed it while cleaning up.
widget/toolbar.go
Outdated
@@ -68,14 +68,102 @@ func NewToolbarSeparator() ToolbarItem { | |||
// Toolbar widget creates a horizontal list of tool buttons | |||
type Toolbar struct { | |||
BaseWidget | |||
Items []ToolbarItem | |||
Items []ToolbarItem | |||
focused bool |
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 don't understand why a toolbarButton should not be focused but the toolbar can be?
This code generally seems strange - why would a bar track which of it's non-focusable children is focused?
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.
In the various apps I have tried it seems that toolbars are not focusable.
If that's not right then we can come back to it.
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.
You are right, but it makes for a lot of Tab presses to navigate through the tab bar. If a toolbar has 10 toolbar buttons, you must press Tab 10 times to go to the first entry in the form. I like it much better with the chosen approach, where the Tab continues to the next object and arrow keys are used to select the actual toolbar button. It is very similar to the tabbed container.
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.
Looking at more applications I see that toolbars are not focusable at all.
It's like they only focus "input controls" and not every element of the interface.
I don't know if that is right either - but if we are to focus toolbar not items then it should probably appeare focused rather than an item in it?
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.
Usualy the toolbar buttons are associated with keyboard shortcuts, and then there is no need to focus them. To do this we need an obligatory shortcut for each toolbar button. I leave this for future updates.
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.
That is a good idea, apart from the mandatory part. Some work for Shortcuts is in place, hopwfully we can make use of that.
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.
IMHO, a toolbar (or containing buttons) never should be focusable. Toolbars are for easy clicking with the mouse. All commands available in a toolbar should also be available as a menu item in the menu bar - hence keyboard-only users already use the menu or its accelerator.
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 agree. But it depends on having keyboard shortcuts. And the menu must be focusable.
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.
It would be great to get this split into smaller pieces so we can see some of this functionality land - the conflict list is probably too long to get this PR landed in one go.
Looks like there is still a format issue in there, plus lots of test failures. |
Some 70 png files had to be changed to implement the new coloring sheme with differnce between focused and primary colors. Modified theme/theme_test.go to check focus color Modified widget/select_entry.go to focus first entry Modified internal/driver/glfw/window.go to not focus buttons on mouse click. Modified: internal/driver/glfw/menu_bar_test.go to unfocus button on start of each test.
Hi @jkvatne I think this would be much easier to review and discuss in detail if you broke it into smaller PRs, this currently contains a lot of different areas of change and in our experience such changes can take exponentially longer than items that focus on one area at a time. There may be a dependency ordering consideration as well. |
Sure, you are correct. I will try to split it more. I was a bit optimistic, but now all tests passes so it is not so bad. |
Please see my proposed new theme API for 2.0. I think this should facilitate the improvements that you reckon we need :) |
Also issue #885 is probably quite relevant |
This PR has a growing list of conflicts as the codebase moves on - what should we do with it @jkvatne ? |
@andydotxyz I think that would definitely the way to go. |
@@ -15,6 +15,7 @@ import ( | |||
"fyne.io/fyne/internal/cache" | |||
"fyne.io/fyne/internal/driver" | |||
"fyne.io/fyne/internal/painter/gl" | |||
"fyne.io/fyne/widget" |
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.
We must not add dependencies from widget
here.
Yes making such an addition will break the theme API so we need to hold it back for 2.0 - but then we can get alt-background and highlighted colours etc |
Radio (-> RadioGroup) and Select widgets have been taken care of by other work. |
There is a lot of good work here and we should take it all into account as we move forward. I'm going to close this, if you find that time becomes available then let's look at how to fill in the blanks of the updated keyboard support. |
Description:
This PR implements keyboard only operation of most but not all widgets. This will improve usability on desktop computers, allowing operation without a mouse.
It passes all tests except the tests where images are compared. The reason is that I had to modify the coloring sheme in order to distinguish between focused and primary colors. The problem is similar to the ongoing changes to implement button animation. What I have called PressedColor should perhaps be named TappedColor.
Checklist:
Where applicable: