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

Cell statusbar API #121215

Merged
merged 13 commits into from Apr 14, 2021
Merged

Cell statusbar API #121215

merged 13 commits into from Apr 14, 2021

Conversation

roblourens
Copy link
Member

@roblourens roblourens commented Apr 13, 2021

Fix #105809

Much better implementation of the cell statusbar API.

Open questions/todos

  • Do extensions need to show items that are only visible when the cell is active? Should I just call the providers again when the cell becomes active?
  • Need to debounce statusbar updates when scrolling?
  • Any other cases when providers should be called? The only case when the provider is canceled is when the item scrolls out of view during an update, is that right?
  • The service is kind of pointless here, would it be better to just expose a list of providers, basically a provider registry?
  • Get other internal things onto this API, and remove statusMessage metadata
  • Fix bugs like Notebook cell status bar lacks left padding #115099 and Notebook cell placeholder text should be updated for Markdown cells #112772

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a simple notebook cell status bar items provider like

	vscode.notebook.registerNotebookCellStatusBarItemProvider({
		viewType: 'mimetype-test'
	}, {
		provideCellStatusBarItems: (cell, token) => {
			return [{
				alignment: vscode.NotebookCellStatusBarAlignment.Left,
				command: undefined,
				text: `${cell.outputs.length} outputs`,
				tooltip: `${cell.outputs.length} outputs`
			}];
		}
	})

and after scrolling up and down a couple of times. I'm seeing duplicated status bar items.

image

image

@@ -99,7 +99,11 @@ export abstract class BaseCellViewModel extends Disposable {
id?: string;
options: model.IModelDeltaDecoration;
}>();
private _lastDecorationId: number = 0;
private _lastItemId: number = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is not intentional, right? it's still a decoration id.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this one is being used for cell status bar. I wonder if we should keep decoration and status bar logic separate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured I'd reuse it, but it doesn't matter, I can keep it separate

@rebornix
Copy link
Member

Need to debounce statusbar updates when scrolling?

It would be great if we debounce it a bit as while scrolling / cell resizing (as they have dynamic height), a cell can be created, disposed and then re-created within a short period.

@roblourens
Copy link
Member Author

a cell can be created, disposed and then re-created within a short period.

What do you mean?

The downside of delaying it is that you will see cells without status bar items briefly, then the status bar items will blink into existence, and I found it annoying when testing.

@roblourens
Copy link
Member Author

Fixed the issue with dupe items showing up, I thought a cancel token would be canceled on dispose by default.

readonly alignment: NotebookCellStatusBarAlignment;
readonly command: string | Command | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mix of ? and | undefined

const unregisterThing = (handle: number) => {
const entry = this._notebookCellStatusBarRegistrations.get(handle);
if (entry) {
this._notebookCellStatusBarRegistrations.delete(handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing dispose?

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only skimmed over the view-parts but what I have seens looks good. Thanks

@jrieken
Copy link
Member

jrieken commented Apr 14, 2021

The service is kind of pointless here, would it be better to just expose a list of providers, basically a provider registry?

Services are always better than singletons, tho that's all they are: better singletons

@roblourens roblourens merged commit 3e40e14 into notebook/dev Apr 14, 2021
@roblourens roblourens deleted the roblou/statusbar branch April 14, 2021 18:26
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2021
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.

None yet

3 participants