Skip to content

Commit

Permalink
Fix progressive rendering issues (#179432)
Browse files Browse the repository at this point in the history
- Tried to do progressive rendering while the view is hidden, this led to calling updateElementHeight with a height of 0 which apparently caused the list to not want to rerender that element later, so now we stop rendering when the view is not visible
- Not restarting progressive rendering when revealing the view again
- Restarting progressive rendering unnecessarily after completing it due to trying to cancel the timer before starting it
  • Loading branch information
roblourens committed Apr 7, 2023
1 parent 817f1b8 commit ea0e3e0
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 7 deletions.
Expand Up @@ -97,6 +97,7 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend
private readonly _editorPool: EditorPool;

private _currentLayoutWidth: number = 0;
private _isVisible = true;

constructor(
private readonly editorOptions: InteractiveSessionEditorOptions,
Expand Down Expand Up @@ -150,6 +151,10 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend
return 8;
}

setVisible(visible: boolean): void {
this._isVisible = visible;
}

layout(width: number): void {
this._currentLayoutWidth = width - 40; // TODO Padding
this._editorPool.inUse.forEach(editor => {
Expand Down Expand Up @@ -239,8 +244,8 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend
throw err;
}
};
runProgressiveRender(true);
timer.cancelAndSet(runProgressiveRender, 50);
runProgressiveRender(true);
} else if (isResponseVM(element)) {
this.basicRenderElement(element.response.value, element, index, templateData);
} else if (isRequestVM(element)) {
Expand Down Expand Up @@ -306,7 +311,14 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend
}
}

/**

This comment has been minimized.

Copy link
@bellaabdelouahab

bellaabdelouahab Apr 7, 2023

I wish I reach this level in coding, maybe one day

* @returns true if progressive rendering should be considered complete- the element's data is fully rendered or the view is not visible
*/
private doNextProgressiveRender(element: IInteractiveResponseViewModel, index: number, templateData: IInteractiveListItemTemplate, isInRenderElement: boolean, disposables: DisposableStore): boolean {
if (!this._isVisible) {
return true;
}

disposables.clear();

let isFullyRendered = false;
Expand Down
Expand Up @@ -74,6 +74,7 @@ export class InteractiveSessionWidget extends Disposable implements IInteractive

private bodyDimension: dom.Dimension | undefined;
private visible = false;
private visibleChangeCount = 0;
private requestInProgress: IContextKey<boolean>;

private previousTreeScrollHeight: number = 0;
Expand Down Expand Up @@ -184,7 +185,12 @@ export class InteractiveSessionWidget extends Disposable implements IInteractive
this.tree.setChildren(null, treeItems, {
diffIdentityProvider: {
getId: (element) => {
return element.id + `${(isRequestVM(element) || isWelcomeVM(element)) && !!this.lastSlashCommands ? '_scLoaded' : ''}`;
return element.id +
// Ensure re-rendering an element once slash commands are loaded, so the colorization can be applied.
`${(isRequestVM(element) || isWelcomeVM(element)) && !!this.lastSlashCommands ? '_scLoaded' : ''}` +
// If a response is in the process of progressive rendering, we need to ensure that it will
// be re-rendered so progressive rendering is restarted, even if the model wasn't updated.
`${isResponseVM(element) && element.renderData ? `_${this.visibleChangeCount}` : ''}`;
},
}
});
Expand All @@ -208,8 +214,11 @@ export class InteractiveSessionWidget extends Disposable implements IInteractive

setVisible(visible: boolean): void {
this.visible = visible;
this.visibleChangeCount++;
this.renderer.setVisible(visible);

if (visible) {
// Not sure why this is needed- the view is being rendered before it's visible, and then the list content doesn't show up
// Progressive rendering paused while hidden, so start it up again
this.onDidChangeItems();
}
}
Expand Down
Expand Up @@ -174,7 +174,7 @@ export class InteractiveRequestViewModel implements IInteractiveRequestViewModel
}

export class InteractiveResponseViewModel extends Disposable implements IInteractiveResponseViewModel {
private _changeCount = 0;
private _modelChangeCount = 0;

private readonly _onDidChange = this._register(new Emitter<void>());
readonly onDidChange = this._onDidChange.event;
Expand All @@ -185,7 +185,7 @@ export class InteractiveResponseViewModel extends Disposable implements IInterac
}

get id() {
return this._model.id + `_${this._changeCount}`;
return this._model.id + `_${this._modelChangeCount}`;
}

get providerId() {
Expand Down Expand Up @@ -294,7 +294,7 @@ export class InteractiveResponseViewModel extends Disposable implements IInterac
}

// new data -> new id, new content to render
this._changeCount++;
this._modelChangeCount++;
if (this.renderData) {
this.renderData.isFullyRendered = false;
this.renderData.lastRenderTime = Date.now();
Expand All @@ -309,7 +309,7 @@ export class InteractiveResponseViewModel extends Disposable implements IInterac
}

setVote(vote: InteractiveSessionVoteDirection): void {
this._changeCount++;
this._modelChangeCount++;
this._model.setVote(vote);
}
}
Expand Down

0 comments on commit ea0e3e0

Please sign in to comment.