search

substance / lens

Lens - open science content creation and display
http://substance.io/lens
MIT License
124 stars 20 forks source link

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

Closed obuchtala closed 9 years ago

obuchtala commented 9 years ago

The problem is, even when the surface selection is rendered at the very end of a change, still a React's component render / forceUpdate is asynchronous. Thus, the selection is rendered before the component.

obuchtala commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

Pls review: https://github.com/substance/substance/commit/cfc7300a87186d60fa3ffc9916b542d74ac1a08f

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 commented 9 years ago

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

obuchtala commented 9 years ago

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

obuchtala commented 9 years ago

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 commented 9 years ago

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

michael commented 9 years ago

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 commented 9 years ago

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

Seems to work.