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

initial focus behavior. #1909

Merged
merged 16 commits into from Mar 9, 2014

Conversation

Projects
None yet
4 participants
@matham
Copy link
Contributor

matham commented Feb 21, 2014

This adds an initial FocusBehavior. It's not complete, and is missing docs. I want to first see if this is in the right direction.

The main thing that needs to be explained is the reason for allowing the user to supply a keyboard using the keyboard property. The reason is imagine having multiple keyboards in the systemandmulti mode. Then as the user tabs through widgets, at each tab, a keyboard is requested and released. This is fairly slow. If the user supplies the keyboard, we don't have to request and release, we just bind and unbind which is very fast. This doesn't seem to be an issue when the keyboard mode is single keyboard.

You can see the large speed difference by running the example code below with and without commenting out Clock.schedule_once(self.late_start, 5). By leaving it in, and clicking on a button and then holding down tab you'll see it's a lot faster than if its commented out.

from kivy.config import Config
Config.set('kivy', 'keyboard_mode', 'systemandmulti')

from kivy.app import App
from kivy.uix.gridlayout import GridLayout
from kivy.uix.boxlayout import BoxLayout
from kivy.uix.button import Button
from kivy.uix.behaviors import FocusBehavior
from kivy.core.window import Window
from kivy.clock import Clock


class FocusButton(FocusBehavior, Button):

    def on_focused(self, instance, value, *largs):
        self.background_color = [1, 0, 0, 1] if value else [1, 1, 1, 1]

    def keyboard_on_key_down(self, window, keycode, text, modifiers):
        if super(FocusButton, self).keyboard_on_key_down(window, keycode,
                                                         text, modifiers):
            return True
        self.text = keycode[1]
        return True

class FocusApp(App):

    def build(self):
        root = BoxLayout()
        self.grid1 = grid1 = GridLayout(cols=4)
        self.grid2 = grid2 = GridLayout(cols=4)
        root.add_widget(grid1)
        root.add_widget(grid2)
        for i in range(40):
            grid1.add_widget(FocusButton(text=str(i)))
        for i in range(40):
            grid2.add_widget(FocusButton(text=str(i)))
        FocusBehavior.autopopulate_focus(grid1)
        FocusBehavior.autopopulate_focus(grid2)
        Clock.schedule_once(self.late_start, 5)
        return root

    def late_start(self, *l):
        keyboard1 = Window.request_keyboard(None, self.grid1.children[-1])
        keyboard2 = Window.request_keyboard(None, self.grid2.children[-1])
        for c in self.grid1.children:
            c.keyboard = keyboard1
        for c in self.grid2.children:
            c.keyboard = keyboard2


if __name__ == '__main__':
    FocusApp().run()
focus_next = ObjectProperty(None)

@staticmethod
def autopopulate_focus(widget, previous=None):

This comment has been minimized.

@akshayaurora

akshayaurora Feb 21, 2014

Member

shouldn't this be moved to add/remove_widget? That way you wouldn't have to iterate over the children list and only set/update the focus_next attribute?

This comment has been minimized.

@tito

tito Feb 21, 2014

Member

I think the "focus" list should not be maintain over add/remove, but be detected by traversing the tree dynamically.

I mean, wherever you are, if you want to go to the next focusable element, you can iterate to the widget tree until you see one of it.

Trying to maintain a list of focusable is hard and should be add/remove widget resistant, which would be only used if the user use focus ability.
Dynamically iterate the tree might cost a little more, but far more easier to write.

In my point of view:

  • In the current implementation: there is no need to have a "focus_previous / focus_next". Instead, we could have method for determine traversing the tree to dynamically determine it.
  • Something might be crutial for the implementation, is the ability to pass a focus_index (same as index= in HTML). If we want to go on that way (dunno if it's preferable or not), then obviously, we need a list of focusable index we can search through it. But while index are shared within an html page, dunno how that would work for us (FocusBehavior static will be always here during the app lifetime, so there is no "index" reset like when you reload a new html page.)
  • Instead of focus_index, maybe use the "focus_next/focus_previous" to let the user control the way he want the focus. If the user set explicitly a focus_next, and if we want to go to the next, use it.

Advices ?

This comment has been minimized.

@matham

matham Feb 22, 2014

Contributor

I agree with tito that it should not be managed by add_widget, but should be dynamic and settable by the user. The way it's implemented now, makes it very flexible, but also very easy for the user to auto-link them.

I actually had in mind to do what tito suggested, but forgot to do it. I added it now. The way it works now is:

  • If the user doesn't do anything, there is no focus order and nothing is linked.
  • The user can link instances by setting focus_next/previous directly, or by using the link_focus function.
  • Finally, the user can call auto-populate which will walk the tree and link all the focusable widgets. However, there's a overwrite parameter, which if False, will leave already set link unchanged.

So, instead of focus_index, we use the focus_next/previous to create the focus order, but the user can set it like they wish, and or autopopulate so that it works out of the box for focus aware widgets.

Hopefully this is the best of both worlds.

else:
self._unbind_keyboard()

def _ensure_keyboard(self):

This comment has been minimized.

@akshayaurora

akshayaurora Feb 21, 2014

Member

This would become redundant if the keyboard is passed arround from the currently focused widget to the widget in focus_next, furthermore since the focus_previous and focus_next props are set through add/remove_widget; this should ensure that a widget out of current visible widget higherarchy is not focued.

This comment has been minimized.

@matham

matham Feb 22, 2014

Contributor

You forget that the user can randomly focus on any instance. Or unfocus. So we cannot assume there will be a keyboard ready.

Even if we would do things through add/remove_widget, that would not affect things later and at any point focus can change randomly. Unless I didn't understand what you meant.

The performance cost of releasing/requesting a new keyboard is minimal when multi is not enabled. Even if multi is enabled, the user should probably have set the keyboard property to some pre assigned groups of keybaords, otherwise a new keyboard will be requested for every focus instance (unless it comes through tab, because then the old keyboard will be released). So I'm not sure it's worth transferring keyboards, instead of simply setting focused to True/False and letting it deal with the keybaord.

But I can do it for the case where focus is set through tab, if you think it's a good idea.

if keyboard not in keyboards:
keyboards[keyboard] = None

def _bind_keyboard(self):

This comment has been minimized.

@akshayaurora

akshayaurora Feb 21, 2014

Member

Same as above. Even the check for disabled/is_focusable can be moved/done while focus_next/prev is being set, no? You would still need to make the binding to the keyboard but the rest would not be necessary anymore.

This comment has been minimized.

@matham

matham Feb 22, 2014

Contributor

That's not quite the case either, because these things can be changed dynamically.

Furthermore, if an instance is linked with other instances using focus_next/previous, setting is_focusable of the widget to False, or disabled to True, should not modify focus_next/previous because at any moment it could become enabled again and we shouldn't have to change focus_next/previous every time it happens.

In fact, even if it's disabled, or is_focusable is False, when auto-populating, we still need include them, because they can become enabled. It's only in on_key_down that we skip disabled widgets.

@akshayaurora

This comment has been minimized.

Copy link
Member

akshayaurora commented Feb 21, 2014

We also need to think about mobile behavior

  1. Since requesting a keyboard launches the soft keyboard which definitely is not desirable on mobile for focusing a button.
  2. It is also desirable in many occasions to release and request a new keyboard on android this resets the soft keyboard and requests a new keyboard that doesn't have the same cache, this is important as the IME/keyboard could be in the middle of compositing text and end up transferring the text being composited(for suggestions) onto the other TextInput.

The mechanisms for moving from one widget to the other on mobiles is quite different, It might just be better to disable focusable support if mobile platform is detected and let the programmer
manage it in his own way.

We could skip widgets that already have a focus property? or work with them like in the case of TextInput... if while adding a widget it has a focus property; add focus_previous/next attributes to it and remove them on remove_widget? (haven't thought this one thourgh).

@matham

This comment has been minimized.

Copy link
Contributor

matham commented Feb 22, 2014

I agree that on a mobile, it is not desirable for a keyboard to pop up every time a widget gets focused. But I am not sure that we should directly disable it. That is partially why I allowed setting the keyboard property so that a user can specify the keyboard, and in that case nothing will pop up when it's focused, since all that will happen is the binding to the existing keyboard.

Also, the user can always set is_focusable to False, if they don't want the keyboard to pop up for some widgets. Because most likely, they would want the keyboard to pop up for some focus widgets, e.g. TextInput. So the user should be responsible for this. However, I also set is_focusable to default to False if not on a desktop.

I did not understand what you said about how keyboards can be transferred suddenly and cache will be mixed up. Or how/what we should do about this. If you give an example of what could happen, I could add a warning to the widget or see how to deal with it.

Regarding TextInput and the like, they should properly inherit from FocusBehavior, because after all that is what it's for. It should be fairly easy to do, i think. The only thing I'm not sure about is the usage of the managed vs. auto mode. I think maybe that's what you had in mind above.

I was split about adding such an option, but I decided for now that the keyboard property is probably sufficient to accomplish what that mode was trying to do. Because when a user sets the keyboard property, that's kinda like managed mode. By allowing managed mode directly, things can become confused, e.g. having focus without a keyboard, or having a keyboard without focus. At that point focus doesn't really make sense (but mobile still has a concept of focus, e.g. multiple textinputs side by side). Also I wasn't sure how someone might actually use managed mode properly.

So I guess, I'm wondering whether the text input mode should be replaced and should use the keyboard property instead. Or if I should add mode (managed and auto) to FocusBehavior?

@matham

This comment has been minimized.

Copy link
Contributor

matham commented Feb 22, 2014

There is two things I'm unsure about.

  • Styling: now, Widget is unaware of FocusBehavior, if we wanted to add a default styling similar t textinput for focused widgets, how would that be done?
  • Does it make sense on multitouch systems for a focused widget to lose focus when a touch is received outside its boundaries? I don't have much experience with mobile, but is it ever possible that there are two users, e.g. with two keyboards sharing the same touch screen, or have two mice, in which case we don't want user 1 touching somewhere having user's 2 focused widget become unfocused. We would only want the user to control their focus. But how could we ever know?
@matham

This comment has been minimized.

Copy link
Contributor

matham commented Feb 23, 2014

Ok, so default styling will not be implemented for now. The last commit adds de-focusing upon escape, so touch won't de-focus (unless it's to focus another widget).

From IRC, it seemed that the plan is to deprecate textinput's keyboard mode once this is accepted and TextInput inherits from FocusBehavior. That should resolve the issues I'm aware of.

@matham matham referenced this pull request Feb 23, 2014

Closed

Focus support for Widget #772

@ToddG

This comment has been minimized.

Copy link

ToddG commented Feb 23, 2014

I implemented my own focus management system on top of kivy 1.8.1. It looks like this:

FocusManager
CursorContainerBehaviour
CursorableItemBehaviour

The FM binds to the keyboard at app startup. As items attain focus, the FM tracks this, and will send subsequent key press events to the widget with focus. Focus is either a traversal from sibling to sibling, or hierarchical as when modal dialogs popup. So the focus state is maintained in a stack, this way the user can nav back down the stack with 'escape' key presses. I added one thing to Widget, which is that widgets bubble keyboard events up (unlike touch events which bubble down).

CursorableItems declare a tab_order, and a tab_container.

It all works quite well, but I'll replace it with your system, once it's baked.

@tito

This comment has been minimized.

Copy link
Member

tito commented Feb 24, 2014

It's cleaner, super cool.

One last point, i would remove the link/unlink focus. Instead, if you want to automatically link them together, you can do:

def on_focus_next(self, instance, value):
    if not hasattr(self, '_old_focus_next'):
        self._old_focus_next = None
    elif self._old_focus_next:
        self._old_focus_next.focus_previous = None
        self._old_focus_next = None
    if value:
        self._old_focus_next = self
        value._old_focus_previous = None
        value.focus_previous = self

Same for on_focus_previous. About using proxy_ref, kv language would do it by default, dunno if we really need to do it here.
There is no self.focus_next set because the value is already assigned when the callback is called. And because of that, we need to track the previous value set in order to unset it.

Edit: Managed to get a smaller impact on the on_focus_next implementation (no more external property required).

False.
'''

focus_next = ObjectProperty(None, allownone=True)

This comment has been minimized.

@tito

tito Feb 24, 2014

Member

You can add baseclass=FocusBehavior, then the user will be able only to set FocusBehavior subclassed widget on it.

This comment has been minimized.

@matham

matham Feb 26, 2014

Contributor

This doesn't work, because FocusBehavior doesn't seem to be defined yet. I get:

   File "C:\kivy-dev\kivy\kivy\uix\behaviors.py", line 421, in <module>
     class FocusBehavior(object):
   File "C:\kivy-dev\kivy\kivy\uix\behaviors.py", line 549, in FocusBehavior
     focus_next = ObjectProperty(None, allownone=True, baseclass=FocusBehavior)
 NameError: name 'FocusBehavior' is not defined
@matham

This comment has been minimized.

Copy link
Contributor

matham commented Feb 26, 2014

  • adds separate widget tree iterators as well as unit tests for them. Changed to use index instead of popping parts of the stack in the algo, saving having to copy the children list.
  • changes the focus behavior to use them, and in the process found and fixed bugs.
  • also squashed both _get_focus_next and previous to a single method, just using the forward and backward iterators.
  • improved the example in examples to be a lot nicer and show how focus works when the layouts themselves can have focus.

I'll remove link/unlink in next commit.

@matham

This comment has been minimized.

Copy link
Contributor

matham commented Feb 27, 2014

  • I removed link/unlink_focus like you mentioned, except I used bind instead of on_focus_next in case the user overwrites without calling super.
  • I also explicitly raise an error if an unfocusable is assigned to focus_next/previous, since baseclass=FocusBehavior doesn't work as mentioned.
@tito

This comment has been minimized.

Copy link
Member

tito commented Feb 28, 2014

What was not working with baseclass?

@@ -767,3 +767,158 @@ def on_opacity(self, instance, value):
:attr:`disabled` is a :class:`~kivy.properties.BooleanProperty` and
defaults to False.
'''


def walk(widget, loopback=True):

This comment has been minimized.

@tito

tito Feb 28, 2014

Member

The implementation look complex for a tree traversal, why you didn't go recursive ?

E.g.:

def walk(widget, loopback=False, index=None):
    # we pass index only when we are going on the parent.
    # so don't yield the parent as well.
    if index is None:
        index = len(widget.children)
        yield widget

    # traverse children
    for child in reversed(widget.children[:index]):
        for walk_child in walk(child):
            yield walk_child

    # if we want to continue with our parent, just do it
    if loopback and widget.parent:
        parent = widget.parent
        # there is a possibily where the parent is incorrectly linked
        if widget not in parent.children:
            return
        index = parent.children.index(widget)
        for walk_child in walk(parent, loopback=loopback, index=index):
            yield walk_child

By the way, i would prefer to have an instance method for that no? Instead of walk(b), use b.walk() ?

This comment has been minimized.

@matham

matham Feb 28, 2014

Contributor

Well, originally, we couldn't do recursive when it was in the behavior, I suppose now we can. But isn't a stack faster and less memory intensive than having python call the functions recursively? And you're right, it should be part of widget itself.

This comment has been minimized.

@tito

tito Mar 1, 2014

Member

For that kind of traversing, you can unroll the recursivity, but i'm not sure about the comprehension / maintenance. For performance, i guess you could try to do a simple benchmark :) (just for fun)

This comment has been minimized.

@matham

matham Mar 1, 2014

Contributor

As I said on IRC, recursion seems slightly faster as tested with https://gist.github.com/matham/9295694. Except when we hit the max recursion depth.

And to repeat, the reason for adding both walkup and loopback to the function parameter list is that in the code you posted, you conflated loopback with walking up. loopback is when we reach the very last element, whether we start back from the tree root. walking up is whether we are allowed to walk up the parent tree from our current level. For recursion to work, there needs to be a walkup parameter, but we also need a loopback parameter for users.

So I guess this means that the recursive path is preferred?

@matham

This comment has been minimized.

Copy link
Contributor

matham commented Mar 2, 2014

Moves it into widget and makes it call recursively. I added a _walk function called from walk to hide the auxiliary parameters (e.g. index) from the user. Also, _walk is needed to stop the loop after a full cycle, otherwise, either we'd loop forever when loopback is True, or we'd need to add yet another parameter to walk holding the original widget, so that we'd know when we made a full circle.

Regarding what wasn't working with baseclass, when I tried it, it told be that FocusBehavior wasn't defined. Presumably, when defining the object property, FocusBehavior is still being built, so it is not yet defined?

When tab is pressed, focus cycles through all the :class:`FocusBehavior`
widgets that are linked through :attr:`focus_next` and are focusable. If
:attr:`focus_next` is None, it instead walks the children lists to find

This comment has been minimized.

@akshayaurora

akshayaurora Mar 2, 2014

Member

Would it not make sense to have this behavior be off by default? i.e focus_next = None means don't go forward/backwards looking for next focusable widget. If focus_next/prev is set to find only then go looking for the next available focusable widget? We could maybe set the focus_next/previous default to find?

This comment has been minimized.

@matham

matham Mar 2, 2014

Contributor

There is no functional difference between what you described and how it is now, other then replacing None by find. In essence, what you call find is now called None.

It would only be different if we change it like you suggested, but then don't default it to find. But that is basically the same if we simply change the default from None to EndIteration. But I don't think that's a good idea, because it should just work without the user having to do anything else. Also, to me, None makes more sense than find, because None to me just means do whatever is default and by default we should be walking the tree.

This comment has been minimized.

@akshayaurora

akshayaurora Mar 2, 2014

Member

It's just that to me EndIteration seems less intuitive. If I want the
focus to not go forward/backward, I'd set focus_next/previous to None.

It just seems If I set focus_next to None and press tab; It would be
highly unintuitive to have the focus still go to the next focusable widget.

setting focus_next to EndIteration to stop focus just sounds off.

What I am proposing is

1.) by default every focusable widget has focus_next/previous set to
'find' or iterate or whatever seems more intuitive.
2.) If I/user set focus_next/previous to None . Just don't walk.

I think None has always equated to 'nothing', 'nada', 'zilch' not the
default behavior

On Sun, Mar 2, 2014 at 8:40 PM, matham notifications@github.com wrote:

In kivy/uix/behaviors.py:

  •        next.focus_previous = None
    
  •    self._old_focus_next = value
    
  •    if value:
    
  •        if not isinstance(value, FocusBehavior) and\
    
  •            value is not EndIteration:
    
  •            raise ValueError('focus_next accepts only objects based'
    
  •                             ' on FocusBehavior')
    
  •        value.focus_previous = self
    
  • focus_next = ObjectProperty(None, allownone=True)
  • '''The :class:FocusBehavior instance to acquire focus when
  • tab is pressed when this instance has focus, if not None.
  • When tab is pressed, focus cycles through all the :class:FocusBehavior
  • widgets that are linked through :attr:focus_next and are focusable. If
  • :attr:focus_next is None, it instead walks the children lists to find

There is no functional difference between what you described and how it is
now, other then replacing None by find. In essence, what you call find is
now called None.

It would only be different if we change it like you suggested, but then
don't default it to find. But that is basically the same if we simply
change the default from None to EndIteration. But I don't think that's a
good idea, because it should just work without the user having to do
anything else. Also, to me, None makes more sense than find, because None
to me just means do whatever is default and by default we should be walking
the tree.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1909/files#r10195753
.

@matham

This comment has been minimized.

Copy link
Contributor

matham commented Mar 2, 2014

As discussed with quanon on irc, instead of defining the EndIteration class to mean stop, we'll use None, and use the 'WalkTree' string to signify to walk the tree.

Also, since we allow now the 'WalkTree' string, it might not make sense to use baseclass anyway, but rather explicitly check the value like we do in e.g. _set_on_focus_next.

@tito

This comment has been minimized.

Copy link
Member

tito commented Mar 3, 2014

I don't agree about the 'WalkTree' solution: the default behavior should traverse the tree, and it make sense to let None as the default behavior no ?
And i don't remember if i was proposing EndIteration, but more StopIteration, which is a standard python exception.

@akshayaurora

This comment has been minimized.

Copy link
Member

akshayaurora commented Mar 3, 2014

@tito The way I proposed it,WalkTree would be the default value and thus
the default behavior. Setting focus_next to None would stop the
iterating/walking. It's just that setting it to None and still having it
walk the tree for next focusable seems counterintuitive.

I guess that if StopIteration is used instead to signify that the focus
doesn't go forward/prev would be intuitive too.
Using None as the default makes sense in a general way.

It's just that I think of the iteration/walking tree part being hidden from
the user and being presented as the default mechanism. It seems intuitive
to try and set the focus_next/previous to StopIteration/None/to user
specified value to stop the default behavior.

I'm not married to either idea though and would actually like a solution
that is more inline with other mechanisms inside kivy.

widget.size_hint_x: None allows user specified height to be used instead of
forcing the default behavior so it seems consistent with that?

Though this is small stuff, we actually need to start paying more attention
to stuff like this to get things more consistent across kivy.

On Mon, Mar 3, 2014 at 3:04 PM, Mathieu Virbel notifications@github.comwrote:

I don't agree about the 'WalkTree' solution: the default behavior should
traverse the tree, and it make sense to let None as the default behavior no
?
And i don't remember if i was proposing EndIteration, but more
StopIteration, which is a standard python exception.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1909#issuecomment-36493803
.

@matham

This comment has been minimized.

Copy link
Contributor

matham commented Mar 3, 2014

I changed to back to what it was before (None means walk tree), except to use Python's StopIteration instead of our EndIteration to mean stop focus.

tito added a commit that referenced this pull request Mar 9, 2014

Merge pull request #1909 from matham/focus
initial focus behavior.

@tito tito merged commit 65c3b42 into kivy:master Mar 9, 2014

@tito

This comment has been minimized.

Copy link
Member

tito commented Mar 9, 2014

Good !

@matham matham deleted the matham:focus branch Apr 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment