Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#16967 closed enhancement (fixed)

Tree: define an overridable method for creating the keyHandlerMap

Reported by: Adam Skwersky Owned by:
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.3
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

This is not a bug but a refactoring request. Consider this code:

var map = this._keyHandlerMap;
				// setup table mapping keys to events
				map = {};
				// The existing handlers
				// The new handlers
				this._keyHandlerMap = map;

In order for derived Tree classes to add to the _keyHandlerMap, we must override the entire method _onKeyDown. Instead the map should be created by a factory method and we should be allowed to override this factory method (calling the inherited factory method to populate existing key presses).

Change History (4)

comment:1 Changed 9 years ago by Adam Skwersky

The reason it is bad to override the _onKeyDown just to add to the map is because there are other parts of that function that may change in future dojo versions. As an example, we did copy this method and added more keys to the map, but in the latter part of the method where it calls the _keyHandlerMap, we were not passing in the event in the message object:

dojo 1.8:

this[this._keyHandlerMap[key]]( { node: treeNode, item: treeNode.item, evt: e } );

our code:

this[this._keyHandlerMap[key]]( { node: treeNode, item: treeNode.item} );

So other methods failed. If we just extracted the map creation to a factory method then we could override just that method and avoid bugs like this.

comment:2 Changed 9 years ago by bill

Component: GeneralDijit
Description: modified (diff)
Milestone: tbd1.9
Resolution: fixed
Status: newclosed
Summary: Tree class should define an overridable method for creating the keyHandlerMapTree: define an overridable method for creating the keyHandlerMap
Type: defectenhancement

Agreed, but _onKeyDown() was already removed completely in [30029]. So this is no longer an issue.

comment:3 Changed 9 years ago by bill

#16977 is a duplicate of this ticket.

comment:4 Changed 9 years ago by Adam Skwersky

Yes, sorry for the duplicate. I think its good that there is now a mixin.

Note: See TracTickets for help on using tickets.