keithamus / jwerty

⌨ Awesome handling of keyboard events
http://keithamus.github.io/jwerty
Other
1.21k stars 109 forks source link

Unbinding should be internalized #61

Closed estaylorco closed 9 years ago

estaylorco commented 9 years ago

Referring to issue https://github.com/keithamus/jwerty/issues/19, returning an unbindable object creates enormous complexity in the client. We now have to internalize jwerty subscriptions in, say, an array or a dictionary.

The better approach would simply be this:

jwerty.unbind('tab', _context);

Internally, jwerty would handle the unbind.

I have written three keyboard navigators with the following class hierarchy:

KeyboardNavigator

KeyboardMatrixNavigator

KeyboardCalendarNavigator

In the matrix navigator, I simply wish to remap up and down to #prevRow and #nextRow, respectively. But I can't really do that without a whole lot of management machinery of the jwerty subscriptions.

In the #bindKeys method of KeyboardMatrixNavigator, where I override the #bindKeys method in KeyboardNavigator, I should simply be able to do this:

KeyboardNavigator.prototype.bindKeys.call(this);
jwerty.unbind('up', _context);
jwerty.key('up', ...)
jwerty.unbind('down', _context);
jwerty.key('down', ...)

Your thoughts?

makoConstruct commented 9 years ago

It doesn't sound like it's necessary in your use case, you could just change what your onUpKey and onDownKey callbacks do according to an external variable, no?

Anyway, it should be implemented. If the _context objects you're referring to are as the return values of jwerty.event, it'd be a one line patch.

If, however, you want the _context objects to be the callbacks, or the context the callbacks are to be called in, or a pair of both of them, we would need a way of storing a mapping from (keycode, _context) to the binding object. There are a few ways of accomplishing this.

estaylorco commented 9 years ago

Since I'm actually mapping about a dozen keys, I don't feel that flags are elegant. They just don't fit the general case.

The _context in this case is the keyboard context (DOM selector). I simply imagined that we would have a compound dictionary key made up of the key combination and the keyboard context. For example, we wouldn't want to accidentally unbind all up keys, perhaps just the up key in a particular keyboard context.

Thinking out loud, I'm seeing a dictionary that might look like this:

[ { { key: keyCombo: 'up', selector: '#someDiv'}, subscription: h } }, 
  {...}, 
  {...}
];

Does that make sense?

makoConstruct commented 9 years ago

If and only if you mean

{
  ['up', '#somediv']: subscription
  ['down', '#somediv']: othersubscription
  ...
}

Though we'd probably store the keys as concatenations of the keycode and the selector rather than an array.

'Fraid that may not be be enough of-popular-interest to land a place on @keithamus's 1.5kb arc?

keithamus commented 9 years ago

If jwerty were to keep a mapping of bound keys, that could cause memory leaks which would be trivial to surface, take for example the following:

jwerty.key('↓', function () {...}, '#myList');
// later on on the code;
$('#myList').remove();

By removing the element, but not clearing up the internal jwerty key binding, jwerty now has a reference to a key binding on a dom element that doesnt exist, but doesn't know about it - therefore a memory leak. The only way around this, to my knowledge, is if we added DOM mutation handlers to every element we bind to - but this is not compatible with lots of browsers and is way too heavyweight.

Currently where jwerty returns an unbind function makes good sense, because there are no references stored in jwerty, it just passes the unbind function - which you have to manage yourself via your own classes, where you have much better knowledge of the application logic than jwerty does.

estaylorco commented 9 years ago

O.K., then. I withdraw my request. I will simply build management around the subscriptions.

Thank you.

keithamus commented 9 years ago

Thanks @estaylorco. Sorry that jwerty can't help you here. If anyone can come up with an idea that wont cause memory leaks, I'll be all ears :smile:.