[editor] Generalise API

[Izhaki]

Currently most of the editor work was done on chip-editor and the whole implementation is very domain specific. We need to start defining the (generic) editor API.

Current Design

The flow for tools/pointer events is like so:

event → tool → (domain) meta → dispatch (with meta) → (domain) reducer

For example (selection tool):

    case 'click': {
      const state = getState();
      const meta = getDomainMeta(action.event.target, state);
      if (meta.selectable) {
        dispatch({ type: 'deselect', metas: state.selected, all: true });
        return next({ type: 'select', metas: [meta] });
      }
      return next({ type: 'deselect', metas: state.selected, all: true });
    }

Goal

We want both tools and actions to be generic, and some domain-specific mapper between them:

event → tool → (domain) mapper → dispatch → reducer

GEF's EditPolicies

EditPolicies take a request from a tool and return a command (to modify the model).

Tool --[request]--> EditPolicy --[command]--> Model

Where are meta's used?

Reducers

Selections

• All can be replaced with ids.
• Because selected items can be either connections or nodes, we will still need to pick the correct action for each selected item (and for optimisation sake probably dispatch a compound action - that is, instead of sending 4 deleteX actions, we send on1 per type.)

markValidPorts

• A reducer (!) that currently manipulate nodes.

isValidConnection()

• Used in connectionTool.

Plan

• Generalise reducers - meaning no domain specific code in them. For now, all domain specific code will move to tools.
• At this point we can move reducers to the editor package and migrate to Redux Toolkit.
• Generalise tools. Where does domain specific code goes exactly? Probably same approach as EditPolicies.

[Izhaki]

We are here

🤔 I do not know for the life of me what's the point in edit policies returning an array of actions that are then dispatched by the tools.
👱‍♀️ Why is it like that?
🤔 Because that's how GEF does it.
👱‍♀️ Why?
🤔 I dunno.
👱‍♀️ So check?
🤔 You check!
👱‍♀️ Seriously.
🤔 That is going to be involved!

---

(A day later)

🤔 Found the GEF source
🤔 If I'm honest, it is really hard to answer that question.
🤔 It seems that it all have to do with OOP bullshit.
👱‍♀️ What do you mean by that?
🤔 Take actions, for example. In GEF the flow will be:

Action → EditPart → EditPolicy → Command → EditPolicy → EditPart → Action (CommandStack.execute)

🤔 Here is some code:

// IncrementDecrementAction.java
private Command getCommand() {
	List editparts = getSelectedObjects();
	CompoundCommand cc = new CompoundCommand();
	cc.setDebugLabel("Increment/Decrement LEDs");//$NON-NLS-1$
	for (int i=0; i < editparts.size(); i++) {
		EditPart part = (EditPart)editparts.get(i);
		cc.add(part.getCommand(request));
	}
	return cc;
}

public void run() {
	execute(getCommand());
}

🤔 Then in the base class:

protected void execute(Command command) {
	if (command == null || !command.canExecute())
		return;
	getCommandStack().execute(command);
}

🤔 So it all seems as if in order to have this base class handling.
🤔 Honestly, can't see any other reason.
👱‍♀️ So that means in our framework edit policies can simply dispatch right?
🤔 Pretty positive.

[Izhaki]

🤔OK, so we are going provide dispatch and getState to edit policies.

Question is how?

Edit Policies API

Options:

Thunk API

Conclusion: OK, but inverted thunk is better.

That is:

(params) => (dispatch, getState) => {}

or in a more compact form:

(params) => store => {}

Familiar.

Notice that we have thunks, like deleteSelected which use edit policies and thunks already get a (dispatch, getState). So if we use (store) we'll have to "reconstruct" it in thunks. What's more - it's not really the store you are getting.

So (dispatch, getState) seems superior here.

Middleware

Conclusion: 👎

store => next => action

But:

• We neither need next
• Nor do we want the API to be based on actions?

Inverted thunk

Conclusion: 👍

store => params => {}

or:

(dispatch, getState) => params =>

This is useful for compound policies (like drags). Where instead of:

    connection: {
      start: event => dispatch => {},
      drag: event => dispatch => {},
      end: event => (dispatch, getState) => {},
    },

We could:

    connection: (dispatch, getState) => ({
      start: event =>  {},
      drag: event =>  {},
      end: event =>  {},
    }),

The difference between the former and the latter is that as far as extracting common policies is concerned, the latter forces reuse of all three sub-policies, whereas with the former you don't have this limitation.

👱‍♀️Will you ever want to reuse one but not the other?
🤔Dunno.
🤔But see discussion about splitting policies below.

We chose this as:

• More consistent policies api between compound (drag) and normal ones.
• Lends itself to split edit policies

Global

Conclusion: 👎

We could apply store into getEditPolicies so each edit policy has closure access to dispatch and getState:

const editPolicies = store => ({
    ...
})

This will allow bindActionCreators.
But this will make it harder to create generic ones.

Splitting Edit Policies

Currently the connection edit policy in chip-editor does 3 things:

• Standard new connection.
• Mark valid connections.
• Undo

We want these split.

Also note that an edit policy might need some context between it subs. For example:

  connection: () => {
    let beforeState;
    return {
      start: ...
      drag: ...
      end: ...
    }

So if we are to split the 3 above we get:

  connection: [
    () => {...}, // Standard connection
    () => {...}, // Mark as valid
    () => {...}, // Undo/Redo
  ]

▶ All this kind of mean that an the inverted thunk api is the right one.

[Izhaki]

First attempt at compound edit policy:

  connection: [
    // Connection
    (dispatch, getState) => ({
      start: event => {},
      drag: event => {},
      end: event => {}
    }),
    // Mark as valid
    (dispatch, getState) => ({
      start: event => {},
      end: () => {}
    }),
    // Undo
    (dispatch, getState) => {
      let beforeState;
      return {
        start: () => {},
        end: event => {}
      };
    }
  ]

The issue is that the connection policy adds the connection and then in undo isValidConnection returns false because the state already includes such connection.

In addition, isValidConnection is used in each of the edit policies (and is called 3 times).

What's interesting is that if we don't have markAsValid we compute these on the fly, but with markAsValid these are computed in advance (so we can cache them).

All this seems to suggest that "reusable" edit policies won't have the (dispatch, getState) API, but rather more specific API, like one that includes isValidConnection.

In turn this suggests that the parent edit policy should be able to do some work before returning the (partly applied) subs.

So this:

  connection: [
    (dispatch, getState) => {
      let beforeState;
      return {
        start: () => {},
        end: event => {}
      };
    }
  ]

Becomes:

  connection: (dispatch, getState) => {
    // Some processing here
    return [
      (dispatch, getState) => {
        let beforeState;
        return {
          start: () => {},
          end: event => {}
        };
      }
    ];
  }

Which will then become:

const undoConnectionPolicy = isValidConnection => (dispatch, getState) => {
  let beforeState;
  return {
    start: () => {},
    end: event => {}
  };
};

const port = {
  connection: (dispatch, getState) => {
    const isValidConnection = () => {};
    return [undoConnectionPolicy(isValidConnection)];
  }
};

Or:

const undoConnectionPolicy = (dispatch, isValidConnection) => {
  let beforeState;
  return {
    start: () => {},
    end: event => {}
  };
};

const port = {
  connection: (dispatch, getState) => {
    const isValidConnection = () => {};
    return [undoConnectionPolicy(dispatch, isValidConnection)];
  }
};

[Izhaki]

Undo

• deleteSelected involves many targets, so each cannot send an undo.
• Same for moveSelected - each will get a move and each cannot do undo.

Probably undo needs to be in the command/tools, but in the case of connection or move - how do we know if something has changed?