scrollable: promptly use displayHight on scroll

[Rosuavio]

During a scroll event, use the updated displayheight over the current displayHight when calculating the new scrollPosition.

Fix #93

[maralorn]

Thank you for your contribution.

I guess having scrollBy and scrollTo be triggered by displayheight was not in scope at time of writing. You could workaround this with a delay, but that might actually cause flickering.

AttachPromptly is always dangerous because of causality loops but I looked into it and since an update in displayHeight is always triggered by an external event from the terminal I guess we are fine.

I will leave this open a few more days so that anyone who actually knows the code base can intervene, but then I would merge this.

[alexfmpe]

*promptly stuff is very rarely actually needed and tricky to ensure it won't later contribute to hard to debug causality loops. It is almost always the case that you can re-work things to make it unneeded. I don't even know when you do need it. Maybe critical performance stuff?

I'm not familiar with reflex-vty but here's what I gather from the diff:

If it makes a difference whether we use attachPromptlyDyn dh scrollTo or attach, it means updated dh triggers at the same time as scrollTo, so we should be able to grab the common ancestor of those two Events and "remember" info with it longer rather than trying to piece things together after the fact, which is what I think is happening here. The propagation of information split at some point into scrollTo and whatever was fed as input to foldDyn or holdDyn to make dh and at this point we try to re-assemble because we believe it fits.

I'd say it's the same pattern as parse, don't validate, only with FRP it's more of a remember, don't discover

[maralorn]

True, however a) as I said, a loop seems quite unlikely in this case b) the code where the "connection gets lost" is in the user code and not in the library. If we want to avoid this bug we should ideally provide an API so that the user can only get the desired behavior. (I.e. make it unlikely to run into this bug by accident.) I am not sure it is possible to design the API like that. It’s even unclear to me whether we can design the API such that the user can have the desired behavior.

So don’t get me wrong. I would love a better solution, but maybe we have to be pragmatic here.

[alexfmpe]

@ali-abrar thoughts?

[ali-abrar]

@Rosuavio if I understand the issue correctly, the promptness is needed when the height is changing in the same frame, and the requested scroll position depends on the new height. Is that right?

[Rosuavio]

@Rosuavio if I understand the issue correctly, the promptness is needed when the height is changing in the same frame, and the requested scroll position depends on the new height. Is that right?

Correct. Does #93, or my usage provide more context?

[ali-abrar]

I think the change makes sense and, while I agree with the sentiment of @alexfmpe's comment, I worry that that would might make it rather difficult to actually use this widget.

Because reflex-vty uses behaviors much more than reflex-dom (which is designed around dynamics), we tend to have fewer issues with cycles.

[ali-abrar]

@alexfmpe are you ok with us moving forward here?

[alexfmpe]

I still think it'd be better to refactor to avoid the promptly, but I don't actually use reflex-vty so I'll defer to those that do

[Rosuavio]

I still think it'd be better to refactor to avoid the promptly

That is something than can be done another time (after this merges) without much aditional breakage, correct?

[maralorn]

I am in favor of merging this as is. No matter, what I am saying next.

I still think it'd be better to refactor to avoid the promptly

That is something than can be done another time (after this merges) without much aditional breakage, correct?

I doubt it, and I am lacking fantasy how that API would even look like.

[Rosuavio]

I meant additional breakage ontop of the breakage that would occure if the refactor was done now.

[Rosuavio]

@ali-abrar no rush, but it seems everyone cleared this.