Feature request: Input map stack

[ghost]

The following code works as expected, but is coupled to the inner-workings of Nodes.java:

  /**
   * This method adds listeners to editor events that can be removed without
   * affecting the original listeners (i.e., the original lister is restored
   * on a call to removeEventListener).
   *
   * @param map The map of methods to events.
   */
  public void addEventListener( final InputMap<InputEvent> map ) {
    nodeMap = (InputMap<InputEvent>)getEditor().getProperties().get( "org.fxmisc.wellbehaved.event.inputmap" );
    Nodes.addInputMap( getEditor(), map );
  }

  /**
   * This method removes listeners to editor events and restores the default
   * handler.
   *
   * @param map The map of methods to events.
   */
  public void removeEventListener( final InputMap<InputEvent> map ) {
    Nodes.removeInputMap( getEditor(), map );
    Nodes.addInputMap( getEditor(), nodeMap );
  }

An API for pushing and popping input maps would be helpful:

  public void addEventListener( final InputMap<InputEvent> map ) {
    Nodes.pushInputMap( getEditor(), map );
  }

  public void removeEventListener( final InputMap<InputEvent> map ) {
    Nodes.popInputMap( getEditor(), map );
  }

In this situation, a StyleClassedTextArea already has an input map and a number of key bindings. My code intercepts all keyTyped() and keyPressed() events until Esc is pressed, to offer the user an inline autocomplete-like mode. When the user presses Esc, the original input map for the editor should be restored. My code shouldn't need to maintain a reference to the editor's input map.

[ghost]

If Nodes could be instantiated using a factory, it would be possible to override its behaviour through polymorphism, while still noting that only one instance must exist. Such as:

interface NodeFactory {
  // Returns a Nodes subclass, always same instance.
  public Nodes newInstance();
}

// e.g., obtained through a factory look-up service.
NodesFactory factory = getNodeFactory();

// Returns a singleton.
Nodes nodes = factory.newInstance();

[ghost]

Another possibility is to offer a delegate:

public interface NodesDelegate {
  public addEventListener( InputMap<?> map );
  public removeEventListener( InputMap<?> map );
}

NodesDelegate delegate = new NodesDelegateImpl();
Nodes.setDelegate( delegate );

[JordanMartinez]

I'm looking at this again.

Essentially, what you want is an API to temporarily swap out one set of InputMaps (e.g. normal/default behavior) with another set of InputMaps for the duration of some special circumstance. Once those circumstances pass, the temporary InputMaps should be removed and the normal one restored. Moreover, you do not want to have to handle the reference to the original InputMap during the swap process, correct?

[ghost]

Moreover, you do not want to have to handle the reference to the original InputMap during the swap process, correct?

Mostly, the following line, which obtains the editor's current input map, should be replaced with an API that is not so tightly-coupled:

nodeMap = (InputMap<InputEvent>)getEditor().getProperties().get( 
  "org.fxmisc.wellbehaved.event.inputmap" );

Generally speaking, the ability to easily switch InputMap instances on the editor would be beneficial. If the calling code needs to retain a reference to the original InputMap, that's fine provided there's a loosely-coupled API, such as:

// Get the current (default) map.
InputMap<InputEvent> originalMap = getEditor().getInputMap();

// Create the replacement map.
InputMap<InputEvent> mySpecialKeyboard = getKeyboardMap();

// Swap out the editor's map.
getEditor().setInputMap( mySpecialKeyboard );

// Restore original map.
getEditor().setInputMap( originalMap );

The calling code can be responsible for restoring previous state.

[JordanMartinez]

While I was working on #18, I saw that Nodes.removeInputMap which calls InputMap.without doesn't remove one portion of an input map without the other portion. Something to be aware of...

[JordanMartinez]

@DaveJarvis Can I get your feedback on the API in my PR?

[ghost]

An absolutely trivially minor part of the code to consider changing is:

node.getProperties().put(...); 
node.getProperties().get(...); 

This is a minor form of duplication and a minor violation of the Law of Demeter. Instead:

node.putProperty(...); 
node.getProperty(...); 

The two methods can use delegation to pass the parameters directly to its internal property map representation.

This protects the calling class from being coupled to the type of map used by the Node instance, allowing the map implementation itself to vary. (For example, perhaps later the Properties can be replaced with a more memory-efficient version, a faster map, or be provided a persistent properties map as a configuration option.)

This doesn't affect the code in any way.

One more item is the method name push/popTemporaryInputMap can be push/popInputMap. The term temporary is subjective here, and potentially misleading to the developer writing the calling code.

[JordanMartinez]

One more item is the method name push/popTemporaryInputMap can be push/popInputMap. The term temporary is subjective here, and potentially misleading to the developer writing the calling code.

Good point. That also makes it shorter so 👍

node.putProperty(...); 
node.getProperty(...); 

I'm a bit confused about what you mean here. There isn't a node.getProperty method.

[ghost]

There isn't a node.getProperty method.

That is the issue. The following lines contain duplicated code:

• https://github.com/FXMisc/WellBehavedFX/pull/18/files#diff-3b30929917e64cb384ab32bc890558a2R110
• https://github.com/FXMisc/WellBehavedFX/pull/18/files#diff-3b30929917e64cb384ab32bc890558a2R115
• https://github.com/FXMisc/WellBehavedFX/pull/18/files#diff-3b30929917e64cb384ab32bc890558a2R119
• https://github.com/FXMisc/WellBehavedFX/pull/18/files#diff-3b30929917e64cb384ab32bc890558a2R121
• https://github.com/FXMisc/WellBehavedFX/pull/18/files#diff-3b30929917e64cb384ab32bc890558a2R125

It's like writing:

• a.getB().c();
• a.getB().c();
• a.getB().c();
• a.getB().c();
• a.getB().c();

This violates the Law of Demeter. (Sometimes playfully stated as, "You can play with yourself, you can play with your friends, but you can't play with your friends' friends.")

This tightly couples knowledge of how a works with respect to its b to any calling class. In concrete terms, it means Nodes needs to reference a Properties instance. Moreover, it means that the underlying implementation would be hard to change in the future.

Instead, drop in a few delegating methods that encapsulate the desired behaviour, rather than leaking it:

public class Node {
  private Properties getProperties() {
    return this.properties;
  }

  /** Delegate to the internal properties map. */
  public Object getProperty( final String key ) {
    return getProperties().get( key );
  }

  public boolean setProperty( final String key, Object value ) {
    getProperties().set( key, value );
  }

  public boolean hasProperty( final String key ) {
    getProperties().contains( key );
    // Or whatever...
  }
}

Then line 125, for example, becomes:

return (Stack<InputMap<?>>) node.getProperty(P_STACK);

Or, simplified even further:

return node.getInputMapStack();

Now how the nodes store properties (and the input map stack) is an implementation detail that doesn't incur unnecessary coupling.

[JordanMartinez]

I believe I understand the heart of what you are saying, which I understand as "code things differently so that, if the underlying implementation changes (say changing how a nodes' properties are stored from a Map to something else), the Nodes API doesn't also have to change to account for that fact. Rather, if we're using a delegating approach, the API accessible to Nodes will remain the same but the internal implementation of the delegating interface can change. Besides that, the node.getProperties() call is duplicate code."

Assuming I've understood you correctly, I am confused for a few reasons.

First, I cannot add the desired public final Object getProperty(String) method to the Node class. After I read through your initial suggestion of node.getProperty(...), I thought, "Surely he's kidding right? How could I add such a method to the Node class?" After pointing that out, you agreed, "That is the issue," the method doesn't exist. However, then your second example (where you clarify yourself) once again seems to assume that I can correct this API issue in the Node class itself. I agree that such an API makes more sense than the current Node API, but I cannot fix that short of submitting a PR to OpenJFX itself. So, when I read through your examples, I am confused because it feels like you are suggesting I do an impossible thing.

The next best thing I could do is add additional methods to the Nodes class that reduce code duplication:

public class Nodes {

 // helper
 private static Map<String, Object> getProperties(Node node) {
  return node.getProperties();
 }

 private static Stack<InputMap<?>> getStack(Node node) {
  return (Stack<InputMap<?>>) getMap(node).get(P_STACK);
 }

 // and similar methods for the other node.getProperties().get(P_[Input/Handler]) calls

 // and then in other methods, I would only write `getStack(node);`
}

Second, I don't see why the "possible change in implementation detail may cause coupling issues" is an issue (short of the code duplication which can be fixed with the code I mention in my first point). If the JavaFX Node API was poorly designed (e.g. there is not node.getProperty(String) method) and I must code things this way as a way to workaround that (e.g. use node.getProperties().get/put(String), it seems the source of the issue isn't my API but theirs. If their API changes to include the node.getProperty(String) method (and other similar ones), it will cause this API to be completely changed. If their API changes so that they handle their internal implementations differently, we would change our internal implementation as well. However, I don't see why or how the current public method signatures (e.g. addInputMap(Node, InputMap<?>) and pushInputMap(Node, InputMap<?>)) would change. If we'd need to change the internals of Nodes, why does that matter?

[ghost]

I cannot fix that short of submitting a PR to OpenJFX itself

Apologies -- I haven't been working with JavaFX for a few months now and did not look to see whether you had control over Node.

possible change in implementation detail may cause coupling issues

The fixes you propose are excellent with respect to the JavaFX API design. The duplication is addressed and the Law of Demeter has been upheld as far as it can.

[JordanMartinez]

Apologies -- I haven't been working with JavaFX for a few months now and did not look to see whether you had control over Node.

Mm... That makes sense.

The fixes you propose are excellent with respect to the JavaFX API design. The duplication is addressed and the Law of Demeter has been upheld as far as it can.

In that case, I'll push the changes and merge the PR.

[JordanMartinez]

Closed by #18.