Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt AbstractTreeRenderer to get re-render behavior #64807

Merged
merged 1 commit into from
Dec 13, 2018
Merged

Conversation

joaomoreno
Copy link
Member

This adopts the AbstractTreeRenderer from master into the two Outline renderers. This base class remembers which elements have been rendered and re-renders them whenever a provided event fires. The event is of type T | T[] | undefined meaning:

  • T only that element will be re-rendered
  • T[] only those elements will be re-rendered
  • undefined every rendered element will be re-rendered

The name AbstractTreeRenderer (and AbstractListRenderer) are pretty bad, but I'm lacking ideas for improving it. I'd like the name to convey the fact that they remember rendered elements and trigger re-renderings based on a given event. Any suggestions?

@jrieken I left a TODO@joh marker for where you can plug in the event which should trigger element re-rendering.

@joaomoreno
Copy link
Member Author

Let's drop this pattern and go with the imperative route. I'll submit a new PR for that.

@joaomoreno joaomoreno closed this Dec 12, 2018
@joaomoreno joaomoreno deleted the joao/outline branch December 12, 2018 07:22
@joaomoreno joaomoreno restored the joao/outline branch December 12, 2018 08:16
@joaomoreno
Copy link
Member Author

I've given it another thought and I keep this proposal. Here's why:

  1. I could open a rerenderElement(index) method in ListView and forward it from a rerenderElement(index) method in the List. Once we reach the level of the tree, the method becomes rerenderElement(tree location) for AbstractTree. In order to transform the latter call into the former, we need to run the getListIndex(tree location) method, which is expensive: it doesn't come for free knowing in which list index a certain tree element is.
  2. So, given that the number of rendered elements is always equal to the viewport size, which is considered to be manageable, the best way to do this is to cache which elements have been rendered. This can be done in many levels: list, tree, renderers.
  3. Implementing this caching at the list and tree level will make every single list and tree spend CPU cycles in maintaining such a cache which, as cheap as it might be, it is still CPU time. This would happen regardless of whether the list/tree user would ever call rerenderElement. It seems wasteful to force all the widgets do work, knowing that they won't benefit from it.

Given that (1) we need that cache and (2) we don't want to waste CPU cycles in trees which will never need to rerender elements, I conclude that implementing this at each renderer level is the best approach.

@joaomoreno joaomoreno reopened this Dec 12, 2018
@jrieken
Copy link
Member

jrieken commented Dec 12, 2018

we need to run the getListIndex(tree location) method, which is expensive: it doesn't come for free knowing in which list index a certain tree element is.

So that them means setSelection, setFocus, reveal, and getRelativeTop are expensive? Esp. setFocus is being used a lot by the outline, e.g when typing in the filter box (focus the best match) or as the editor changes selection (follow cursor). This happens much more frequent than re-render (need config change or updated markers). Is there something else that I should use then?

@joaomoreno
Copy link
Member Author

That's a great point. We simply can't have O(1) on all the list/tree methods. In the current implementation, any method which needs to know the list index of a specific tree element will have some cost to it. My longstanding bet is that these methods are called punctually, never in a loop.

Let's say we introduce rerenderElement. You happen to know exactly which elements need rerendering from your event. I don't think that will be the case for everyone. For those other folks, when their event fires, they are forced to iterate and call rerenderElement() on every element. Suddenly they are calling one of those costly methods in a loop. We definitely want to avoid that.

The cache makes sure that not only we iterate at most only on the visible elements but also that we do not need to know at all any list index when the time comes to rerender.

If you are worried about the use of setFocus et al, let's profile it and track that in an issue if we find it considerably affects the frame rate.

@jrieken jrieken merged commit aa310b3 into joh/outline Dec 13, 2018
@jrieken
Copy link
Member

jrieken commented Dec 13, 2018

Let's see how far I get with this

@jrieken jrieken deleted the joao/outline branch December 13, 2018 09:48
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants