Added cachedScrollTop to SCM panel

[babakks]

Fixes #73622

Added the new private property cachedScrollTop (to RepositoryPanel) to hold the vertical scrollbar position when its visibility changes.

[babakks]

The first commit seemed illogical since it was handling rendering concerns (scrollbar position) outside the layoutBody() method. So, I moved that part.

More that that, when the user resizes the sidebar, the layoutBody() method is called repeatedly which makes the scrollbar flash/spark when the user releases the grip. Therefore, the cached scroll position needs to be invalidated after every usage. This is also done in the last commit.

[joaomoreno]

The fix might be much simpler than this. Other viewlets keep their scroll position as they come and go, and they don't save any scroll top. We have to see why SCM is different and fix that instead.

[babakks]

Let me check then.

[babakks]

The SCM viewlet

As I discovered, other viewlets virtually do nothing to keep their scroll position.

Actually, the unique behavior of the repository panel (RepositoryPanel) in the SCM viewlet, in comparison with the Explorer and other viewlets, is that it removes/adds the visual elements (i.e., list items) once its visibility changes: (scmViewlet.ts:865)

private onDidChangeVisibility(visible: boolean): void {
  if (visible) {
    const listSplicer = new ResourceGroupSplicer(this.repository.provider.groups, this.list);
    this.visibilityDisposables.push(listSplicer);
  } else {		
    this.visibilityDisposables = dispose(this.visibilityDisposables);
  }
}

Note that the dispose() method called in the else block, ends up calling the ResourceGroupSplicer's dispose() method (shown below), which in turn removes all list items both from its own context (i.e., the ResourceGroupSplicer's items property) and from the RepositoryPanel's list property: (scmViewlet.ts:684)

dispose(): void {
  this.onDidSpliceGroups({ start: 0, deleteCount: this.items.length, toInsert: [] });
  this.disposables = dispose(this.disposables);
}

Therefore, it's quite normal for the list to reset the scroll position to the top since all its items are removed. In other words, it's not losing any view state. Other viewlets do not clear their list/tree items, so their view state is retained throughout the entire application's life cycle.

Retaining items

To verify this, I changed the event handler above to the one below so as to avoid disposing of visuals:

private isCreatedOnce = false;
private onDidChangeVisibility(visible: boolean): void {
  if (!isCreatedOnce && visible) {
    const listSplicer = new ResourceGroupSplicer(this.repository.provider.groups, this.list);
    this.visibilityDisposables.push(listSplicer);
    this.isCreatedOnce = true;
  }
}

and it did keep the scroll position from resetting.

Another instance

The Outline view (tree), too, disposes of visual elements but since the data trees provide their view state, which merely contains selections and expanded/collapsed state of the tree items (not the scroll position), once revealed, the viewlet retrieves the last view state and seems to retain the scroll position. But when there's no selected item, the Outline viewlet scroll keeps reseting to the top.

TL;DR

At this moment, I see these solutions:

1. Using a special/ad hoc solution specific to the SCM viewlet that keeps score of the scrollTop property (current commit).
2. Avoid disposing of visual elements when the SCM viewlet hides (second handler above).
3. Enable WorkbenchList<T> to expose its view state (for example, through a settable viewState property).

IMO, the second solution needs further study as to discover why it was decided at first to remove the list items. The third solution requires design-level decisions on, presuming it being acceptable, what view state parameters to expose and how it should be done. So, I'm still left with the first choice.

[joaomoreno]

Therefore, it's quite normal for the list to reset the scroll position to the top since all its items are removed. In other words, it's not losing any view state.

Absolutely agree. Great investigation! 👏

We'll take it, thanks! 🍻

[joaomoreno]

👌

[babakks]

Thank you for making me go for that. 👍