Extract transformations from tools

[michael]

This is a work in progress. Opening pull request for discussion.

[michael]

Oliver can you have a look at the new toggleAnnotation transformation. I think there's some potential to further improve this. Maybe you have some proposals.
For instance I don't like the API of the checkers canCreate etc. I always pass annos, which contains all annos that are matching the currentSelection and a certain type. I do this to not have to recompute them on every check.

We had way too much logic encoded into our tool implementations, so it's good we are tackling this now. What's nice is that we have a toggleAnnotation transformation now which does not depend on any UI stuff. We just parametrized everything (for instance containerId to be able to create new container annotations).

While this implementation is not super clean yet, it seems to work again, and I would like to go one step further and introduce named commands that can be executed using surface.execCommand. So for instance when a tool has been clicked we can just call this.surface.execCommand('toggleStrong'). A keybinding would do just the same. Using this approach we can register commands on the surface (e.g. passing a commands hash via constructor) and don't need any ui components for it. the tools would just silently hook in.. use some API to determine toolstates but simply executes the named command, which would silently die if the command is not registered.

If i introduced a command class, it could allow it to access the surface, e.g. to determine things like the current container etc to parametrize the corresponding transformation accordingly. Maybe it would also make sense to move the canCreate canUpdate stuff into the command context, and instead of implementing a toggleAnnotation transformation, which is super highlevel, we could implement createAnnotation, truncateAnnotation, expandAnnotation etc. transformations which maybe useful to combine in other scenarios. Also then the tranformation interface would be a simple function... without all the helpers (canCreate etc.).

Speaking of helpers.. all the helpers used by toggleAnnotation, that just read information... maybe they should also get their dedicated place for being reused. getPropertyAnnotationsForSelection for instance. It's too bad these things are locked inside the toggleAnnotation transformation.

[obuchtala]

What you said makes sense. I think it is good to extract things step by step. Go ahead.

For instance I don't like the API of the checkers canCreate etc. I always pass annos, which contains all annos that are matching the currentSelection and a certain type. I do this to not have to recompute them on every check.

Getting rid of these is not so easy. I would not change it for now.

Speaking of helpers.. all the helpers used by toggleAnnotation, that just read information... maybe they should also get their dedicated place for being reused. getPropertyAnnotationsForSelection for instance. It's too bad these things are locked inside the toggleAnnotation transformation.

You can put helpers onto document, if you think they fit there.

I think the next step as you said is introducing a solution, so that you can execute things via key-bindings. Surface is a good context for that.

Let's look at it after this step, and then decide how to go on.

[michael]

Thanks Oli. Question: There are no indexes on a transaction document, however I need them in my transformation for the checkers. Is it ok to just use the .document reference? See here: https://github.com/substance/substance/blob/extract-transformations-from-tools/document/transformations/toggle_annotation.js#L286

Not sure if this can be solved better.

[obuchtala]

We should have indexes actually.

[obuchtala]

https://github.com/substance/substance/blob/master/document/transaction_document.js#L20

[michael]

But not for container annotations.

[obuchtala]

This is actually related to my current refactor. And I can't provide you a solution by then.
You can use tx.document, but consider it as a hack. And don't expect this index to get updated during the transaction.

[michael]

Ok! It was previously performed outside of the transaction, but now that we want to encapsulate toggleAnnotation as a transaction it has to live there too. I'm still thinking of getting rid of the checkers in the transformation and do this on the Command level. However still.. if then I have truncateAnnotation as it's own transformation, it should do the check for canTruncate I guess. Not sure though.

What is your general opinion here, should transformations be rather dumb data operations, that do their thing? A lot of things are actually domain specific behavior, e.g. that strong should be fused when possible etc. while a different type of operation should not. I have a feeling that having them in the transformation is the wrong place.. also with respect to customizing the behavior.

[obuchtala]

I agree that this should not be decided by the transformation.

[michael]

Will try to move it into this new Command scope. But first deliver Erste.

[michael]

Could you review this briefly?

75a4358

I introduced a Command interface. Unlike transformations commands are classes so we can use our nice override mechanism that we had with tools. Tools now got a lot dumber, they just depend on a command and use instance methods from there. I made truncateAnnotation, expandAnnotation etc independent transformations, as they may be useful in general.

Commands would be CLI-able... however the ToggleAnnotationCommand I implemented would need a surface-ish instance to be passed on construction.

[michael]

Info: I combined all the work here into a new single commit.

[michael]

Should we consider introducing parameters for commands? I'm thinking about how I could refactor the switch text tool.

Either I define new commands for each action:

surface.executeCommand('makeH2');
surface.executeCommand('makeCodeblock');

Or I allow params for commands.

surface.executeCommand('switchTextType', {type: 'heading', level: 2});
surface.executeCommand('makeCodeblock');

Not completely sure what implication this has.. e.g. when we map keyboard commands the handlers will need to construct the commands complete with params. What's your feeling?

[michael]

With params we could maybe better avoid implementing new CommandClasses in many cases. Just not sure if it's worth it. For instance I could spare all those ToggleStrong, ToggleEmphasis implementations and do a generic:

surface.executeCommand('toggleAnnotation', {type: 'strong'});

However for link for instance it would already not work, because we need do provide some additional properties like the url on creation, and then do this open dialog thingee.

surface.executeCommand('toggleLink');

I don't know.. some feeling tells me that we should just write more command classes instead of being smart.

[michael]

I decided to go without command params. Will commit refactored switch_text_type tool soon....

[obuchtala]

I can't say right now. Give it a go and let's look at it afterwards.

[michael]

Please review: 825426f

The available text types are now configureable. You can specify for a surface which commands you want to have supported. The text type tool now asks the Surface for the available text type commands and render them in the dropdown.

This is close to being ready for merge. Only LinkTool has not been ported yet. Consider rebasing your work in progress to this branch, it should be safe!

[obuchtala]

👍 Merge it after rebase.

[obuchtala]

Make use of new OO features.

[michael]

Rebased. I will merge tomorrow after i have the link tool integrated.

[obuchtala]

NP

[michael]

this was somewhat a heavy job. 😅

but more or less i like it like that. Command replaces Tool more or less.

[obuchtala]

Yeah. Well done! I like that you take over :)

[michael]

;)

Once this is merged, can you take care of bringing in keybindings? I'd then head over to to my writer refactor task. And get our improved Component under some load. :)

[obuchtala]

What about heading 3+? I assume, we will add this later...

[michael]

tomorrow! :) 1,2,3 is enough right?

[obuchtala]

yeah yeah....

[michael]

Please review and merge.

[obuchtala]

Looks good. Can I see it in action somewhere?

[michael]

substance/demos, writer branch should do. Hope i did not forget to commit.
But will be home soon.

On Di., 15. Sep. 2015 at 22:58 Oliver Buchtala notifications@github.com
wrote:

Looks good. Can I see it in action somewhere?

—
Reply to this email directly or view it on GitHub
#43 (comment).

[obuchtala]

👍

[michael]

I have to fix the notepad demo, it also needs to use the new command API.