Rebinding a KeyHandler should create a new instance instead of replacing the callback

[kasper]

Rebinding a KeyHandler should create a new instance instead of replacing the callback to ensure a clean state for the new handler.

• Version: 2.1.1

[kasper]

@mafredri @ontucker @jasonm23 I think I’ll fix this in the next minor version as this is more of a design flaw than a bug. As this will make some breaking changes to the API, I wanted to get your opinions. How about this:

1. Key handlers will always be enabled by default.
2. All key handlers will have unique hashes even if they consist of the same key combinations. This is the same behaviour as with event and timer handlers.
3. Subsequently, you can then have multiple key handlers for a key combination, but only one will be enabled at a time.
4. If a key is bound with a previously existing key combination, the latest active handler will be disabled, but the user is still responsible for releasing the reference.
5. Key handlers can be enabled and disabled as desired, but only one can be active for a key combination at a time.

Would this make sense?

[mafredri]

What you described is pretty much what I'd expect the behavior to be, makes perfect sense to me 👍.

[ontucker]

Me too. One point in (4) is that it shouldn't be an error to disable a handler which has, unbeknownst to you, been disabled by another handler with the same key combination being enabled overtop of it.

One thing unspecified here is whether the key handlers for a given key combination are effectively like a stack where last-enabled wins, but if you disable that, the previously-enabled one gets re-enabled again. That could simplify things for modules which want to temporarily override key combinations and then put things back the way they were without the owners of those combos having to do anything.

Note that it might also make sense to have a keyhandler emit an event when it becomes disabled due to another one stomping on it, so the owner knows that the handler is inactive.

[kasper]

@ontucker Good points! It’s currently OK to disable a key handler that has been already disabled. Nothing will happen, but the API will return true. This shouldn’t change. Emitting events about the state changes is definitely a good idea if a new handler disables a previous one.

The problem with implementing a stack-like behaviour is that Phoenix currently has no idea when the user releases the references. This all happens inside the JavaScript’s scope. So most likely this behaviour should be the user’s responsibility,

This leads to another idea, should new key handlers that have the same key combinations as active handlers in fact be disabled after creation? Would this be more intuitive? This way nothing unexpected would happen.

[ontucker]

I have to admit, I find the "key handler is disabled when the user releases the reference" behavior a bit odd and un-javascript-ish. In most javascript event APIs, the handler stays attached whether you retain a reference to it or not. If you had this model, Phoenix wouldn't need to care whether the user had released references -- the handler would stay active until it's explicitly removed or superceded. I acknowledge you may have internal architectural reasons this model is more desirable, though.

[kasper]

@ontucker This is defined by two reasons: 1.) how memory management is handled in JavaScriptCore — this states that Objective-C cannot retain JavaScript callbacks, otherwise we will end in a retain cycle (ARC vs. garbage collection). It also requires that the JavaScript context keeps a reference to the object. When this reference is released JavaScriptCore knows it can finally garbage collect the object. This abstraction also means, the general runtime knows very little when objects are actually released. 2.) An architectural desicision that keeps Phoenix extremely lightweight.

Obviously, this all is completely internal behaviour partly demanded by memory management. We could implement a wrapper within the JavaScript context to take care of keeping the references alive and abstract this from the user. You could in fact do this yourself. This is what other JavaScript libraries (or rather browsers/runtimes) do.

Obviously this is quite language technical, but someone always needs to take care of the object lifecycle.

In fact, currently Phoenix doesn’t know or care when the user releases the handlers and the behaviour you described would need the complete opposite if it were to be implemented in Objective-C runtime. :)

[kasper]

You could abstract it like this: #90 (comment).

[kasper]

@ontucker #107

[kasper]

This has now been implemented in master as per the description above and will be released in 2.2.

@mafredri @ontucker If you have time, I would really appreciate if you would test this change in your advanced contexts. Thanks! 👍

[mafredri]

Spectacular! I'll try to have a go at this today :)!

[mafredri]

Except for one ?-mark, it works pretty darn well! I noticed that as long as a key handler is enabled, re-enabling another one (without creating a new one) cannot override the currently active key handler. Was this intentional?

For example, this doesn't work:

var a1 = Phoenix.bind('a', [], function () { Phoenix.log('a1'); });
var a2 = Phoenix.bind('a', [], function () { Phoenix.log('a2'); });
var currentA = a2;
var at = Phoenix.bind('a', ['shift'], function () {
    if (currentA === a1) currentA = a2;
    else currentA = a1;
    currentA.enable();
});

Whereas modifying the toggle function like so, works:

var at = Phoenix.bind('a', ['shift'], function () {
    currentA.disable();
    if (currentA === a1) currentA = a2;
    else currentA = a1;
    currentA.enable();
});

[kasper]

Great! I presume the first example logs an error while enabling currentA? Unfortunately, garbage collection is delayed, so the previous handler most likely won’t be disabled in time for enabling the second one. That’s why disabling is a safe bet.

[kasper]

Oh, and indeed only binding will automatically disable the previous active handler. Enabling and disabling existing handlers is manual.

[mafredri]

Great! I presume the first example logs an error while enabling currentA?

Yup!

Unfortunately, garbage collection is delayed, so the previous handler most likely won’t be disabled in time for enabling the second one. That’s why disabling is a safe bet.

Ah, there is actually no possibility for garbage collection in my example, but yes, there would usually be a slight delay if that was the case :).

Oh, and indeed only binding will automatically disable the previous active handler. Enabling and disabling existing handlers is manual.

That's what I though, was just confirming if it was intentional or not 👍.

[kasper]

Great! Yes, that’s intentional. I was also debating whether binding an existing key combination would result in a handler that is disabled, but probably in most cases that would not be desired.

[mafredri]

I think I agree, disabling an enabled handler when another is enabled would likely lead to hidden logic bugs and unpredicted behavior. Better to fail early :)

[kasper]

@mafredri Actually, forget that. I did a significant refactor in #109 which simplified the logic drastically. Enabling will now disable the previous handler in every case. 😄

I would appreciate, if you could test this again!

[mafredri]

Wow, you've been busy! I'll have to update my type definitions :D.

But yeah, works perfectly! Now there's no problems with my "this doesn't work" example code. This got me thinking, do you think it would be feasible to have an api for getting the currently active key handler as well? E.g. Key.handlerFor('a', ['shift'])? Something I think I might have use for when having different modes and mixing Key.on(...) and new Key(...). For example, a module could be quite self-contained if it was able to temporarily enable its bindings, and when it is deactivated, re-enable whatever was active before it.

[ontucker]

Ooh, that's a great idea @mafredri. I pass my modal modules a list of keybindings they should disable when they're enabling their own, but it would be really nice if they could build that list themselves by querying for existing bindings on the keys they want.

[kasper]

Brilliant! Regarding getting the current active handler, it’s possible for the managed API, but not for the unmanaged. Phoenix doesn’t anymore keep reference to all keys to simplify things.

You probably should manage this yourself. As @ontucker previously mentioned, an option would be to post an event about when a key is enabled or disabled.

[mafredri]

Alright, I guess it could be a bit confusing to have that only for the managed API. Say a key is enabled through an unmanaged handler, and there's also the same key available as a managed handler (disabled), then this function would not return anything since the (managed) handler isn't enabled.

[kasper]

@mafredri Indeed. :)