React-Race-Condition Problem: Rerendering the selection on a text node which is just created gives error about lens

[obuchtala]

Actually, what we see is just our own warning...
But in general this is a undesired situation.
ATM, we trigger surface selection rerendering from within the text-property component after rendering, which works.

However, maybe there is a better solution?

[michael]

I still don't understand the situation, i was poking in the code but could not find the environment that causes that warning. Where is this triggered in the component? i guess that's a textproperty component thing?

[michael]

Ok, I think i got it now. Somehow it would be nice when the textproperty knows nothing about selection rerenderings. So i would tend to remove that explicit call there? Instead maybe in Surface._setModelSelection we should make a setTimeout this.rerenderDomSelection, so we make sure the ui is up to date?

Just as a general idea.. i'm probably overlooking something...

[michael]

Pls review: substance/substance@cfc7300

I think we should inverse the control and let only the surface call rerenderDOMSelection but give it a change to know when this is necessary. E.g. when some text texproperty has been updated, but then only if the selection is affected there should be a rerender dom selection.

[obuchtala]

Do you see the warning on the console?
You can click on the location link... then u know where it is done.

[obuchtala]

Sorry... last comment was probably not up2date anymore.

[obuchtala]

Yeah... the timeout thing we used to do before.
I suppose it is as good as any solution could be... considering concurrent situations where u type rapidly
the pure fact of render being async could possibly cause troubles...
But I think this is not a real problem for us, as the only situation where we pull the selection from DOM is after cursor navigation and mouse click.
During typing we just rerender and maybe causing a wrong cursor for a tiny bit of a second.

[obuchtala]

And hopefully the async render is more like setTimeout(0), otherwise we would have to delay longer.

[michael]

Yeah we need to investigate that a bit more.. but i'm relatively certain it's the setTimeout(0) thing.

Maybe we could describe the rerender necessity in a condition. E.g. the surface could listen to document changes and determine wether a selection rerender is necessary or not.

Not sure how hard it is to implement such a checker generically:

selectionIsAffectedByDocChange(docChange, sel) {}

[obuchtala]

We don't do rerender from TextProperty anymore... instead Surface.rerenderSomSelectin() does it delayed.

Seems to work.