t/50: ToggleAttributeCommand should correctly determine its initial state

[oleq]

Suggested merge commit message (convention)

Fix: ToggleAttributeCommand should correctly determine its initial state. Closes ckeditor/ckeditor5#2872.

[Reinmar]

the current

[Reinmar]

I'd say that we don't have to link to this issue here. This should be the standard way of handling refreshing.

[Reinmar]

Sounds like we need CommandCollection finally :)

[oleq]

Do you know other places where we iterate over them in a similar way?

[Reinmar]

We'll do so on destroy (we don't destroy commands now). So, I'd rather see these methods (destroy() and refreshAll() in the CommandCollection).

[Reinmar]

I've just realised something – why do we need to refresh commands on dataReady if they are supposed to be refreshed on changesDone? I got confused now :D

The only situation I can see here, where refreshing on changesDone would not work is if an editor creator would not load initial data (e.g. if the editor should be empty initially). But, I think this needs to be handled in the engine, so there's always initial changesDone fired.

[Reinmar]

BTW, there's one more reason why this is a better option – notice that the solution we proposed (calling cmd.refreshState() on dataReady) does not refresh the value...

So far, not every command needs a value so refreshValue() was not defined as Command's method. It's an optional method which child classes often define. Perhaps we could change this and have refreshState(), refreshValue() and perhaps refresh() which calls them both, but there's a problem that refreshState() is not to be overridden actually. It's the public method whilst the protected _refreshState() is supposed to implement all the logic. We'd need to go the same way with refreshValue() to be consistent... Or we may have public refresh() and protected _refreshValue() and _refreshState()... This may be an option.

Anyway, by making changesDone a reliable hook we move the duty to refresh themselves to commands. And this is better – only commands should be concerned about this.

[oleq]

I removed the Editor part of the fix and created a PR in the ckeditor5-link.

[Reinmar]

For the future – such issues should have some new integration tests. I think that the existing ones were a bit poor, hence, the issue.