VSCode: add status bar

[marcuspang]

Fixes: #6474

Changes

• add status bar component
• add cairo1.showInStatusBar config item in vscode settings
• made status bar component clickable to LSP logs

Let me know if more info should be added to the UI.

Screenshots

Hovering over the extension shows the following info:

Enabling/disabling the setting works:

Screen.Recording.2024-10-17.at.14.47.26.mov

[lotem-starkware]

This change is 

[mkaput]

Reviewed 1 of 3 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @marcuspang, @mkaput, @orizi, and @piotmag769)

a discussion (no related file):
🤩

---

---

vscode-cairo/package.json line 144 at r2 (raw file):

            "description": "Show Cairo Language Server information",
            "scope": "window"
          }

I did a quick search in popular extensions and these name & description pattern seem to be a convergent style used by most of them.

Suggestion:

          "cairo1.showInStatusBar": {
            "type": "boolean",
            "default": true,
            "description": "Show CairoLS information in the status bar.",
            "scope": "window"
          }

---

vscode-cairo/src/context.ts line 12 at r2 (raw file):

      }),
    );
    const statusBarItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left, 100);

Consider encapsulating all the logic and state in a StatusBar class, that is:

1. Move statusBarItem instance inside it, and keep StatusBar instance as a field in this Context. You should be able to use definite assignment assertion to resolve cyclic dependency.
2. Make setupStatusBar etc. plain methods there.

---

vscode-cairo/src/extension.ts line 24 at r2 (raw file):

    ctx.log.warn("status bar is disabled");
    ctx.log.warn("note: set `cairo1.enableStatusBar` to `true` to enable it");
  }

imo no need to pollute logs about this

Suggestion:

  if (ctx.config.get("enableStatusBar")) {
    await setupStatusBar(ctx, client);
  }

---

vscode-cairo/src/statusBar.ts line 40 at r2 (raw file):

    try {
      const scarb = await Scarb.find(vscode.workspace.workspaceFolders?.[0], ctx);

We will probably want to query CairoLS (via custom request type) because it might have more context than the extension. But this is definitely a topic for separate PR, though. I'll make a separate issue for this.

[mkaput]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @marcuspang, @orizi, and @piotmag769)

[marcuspang]

Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/package.json line 144 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I did a quick search in popular extensions and these name & description pattern seem to be a convergent style used by most of them.

Done.

---

vscode-cairo/src/context.ts line 12 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Consider encapsulating all the logic and state in a StatusBar class, that is:

1. Move statusBarItem instance inside it, and keep StatusBar instance as a field in this Context. You should be able to use definite assignment assertion to resolve cyclic dependency.
2. Make setupStatusBar etc. plain methods there.

Done.

---

vscode-cairo/src/extension.ts line 24 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

imo no need to pollute logs about this

Done.

---

vscode-cairo/src/statusBar.ts line 40 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

We will probably want to query CairoLS (via custom request type) because it might have more context than the extension. But this is definitely a topic for separate PR, though. I'll make a separate issue for this.

Gotcha Ill copy over the TODO in cairols.ts

[mkaput]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @marcuspang, @orizi, and @piotmag769)

---

vscode-cairo/src/extension.ts line 20 at r4 (raw file):

  if (ctx.config.get("showInStatusBar")) {
    ctx.statusBar.setupStatusBar(client);
  }

setupStatusBar does this internally so we don't have to repeat this condition here

Suggestion:

  ctx.statusBar.setupStatusBar(client);

---

vscode-cairo/src/statusBar.ts line 40 at r2 (raw file):

Previously, marcuspang (Marcus) wrote…

Gotcha Ill copy over the TODO in cairols.ts

https://github.com/starkware-libs/cairo/issues/6502

---

vscode-cairo/src/statusBar.ts line 16 at r4 (raw file):

  }

  public setupStatusBar(client?: lc.LanguageClient): void {

Suggestion:

public setup(client?: lc.LanguageClient): void {

---

vscode-cairo/src/statusBar.ts line 39 at r4 (raw file):

  }

  public async updateStatusBar(): Promise<void> {

this doesn't have to be public

[Draggu]

Reviewed 1 of 3 files at r1, 2 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @marcuspang, and @piotmag769)

[marcuspang]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/extension.ts line 20 at r4 (raw file):

Previously, mkaput (Marek Kaput) wrote…

setupStatusBar does this internally so we don't have to repeat this condition here

Done.

---

vscode-cairo/src/statusBar.ts line 16 at r4 (raw file):

  }

  public setupStatusBar(client?: lc.LanguageClient): void {

Done.

---

vscode-cairo/src/statusBar.ts line 39 at r4 (raw file):

Previously, mkaput (Marek Kaput) wrote…

this doesn't have to be public

Done.

[mkaput]

waiting for @Arcticae review for second pair of eyes on TS side of things

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @orizi, and @piotmag769)

[Arcticae]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @marcuspang, @orizi, and @piotmag769)

---

vscode-cairo/src/context.ts line 23 at r5 (raw file):

  public readonly config: Config = new Config();
  public statusBar!: StatusBar;

I don't know why it's a non-nullable reference if it's not enforced (Context is created and only later is the statusBar being set

---

vscode-cairo/src/statusBar.ts line 20 at r5 (raw file):

      vscode.workspace.onDidChangeConfiguration((e) => {
        if (e.affectsConfiguration("cairo1.showInStatusBar")) {
          this.update();

This should figure out if the change is to hide or show it (and perform it), because it needlessly queries for version and looks for scarb once again

---

vscode-cairo/src/statusBar.ts line 43 at r5 (raw file):

    const showInStatusBar = config.get<boolean>("showInStatusBar", true);

    if (showInStatusBar) {

This function probably should be split into:

1. Looking for scarb
2. Refreshing the version
3. Showing/hiding the statusbar

Right now it does a bit much

---

vscode-cairo/src/statusBar.ts line 61 at r5 (raw file):

    } else {
      this.statusBarItem.hide();
    }

Suggestion:

      this.showStatusBarItem();
    } else {
      this.closeStatusBarItem();
    }

[marcuspang]

Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/context.ts line 23 at r5 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I don't know why it's a non-nullable reference if it's not enforced (Context is created and only later is the statusBar being set

I think what you said made sense. I decided to move the creation to the constructor. Not sure if it would be better to take it in as an optional parameter in the constructor instead

---

vscode-cairo/src/statusBar.ts line 20 at r5 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This should figure out if the change is to hide or show it (and perform it), because it needlessly queries for version and looks for scarb once again

Gotcha, I remove this check, update() still checks it when called

---

vscode-cairo/src/statusBar.ts line 43 at r5 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This function probably should be split into:

1. Looking for scarb
2. Refreshing the version
3. Showing/hiding the statusbar

Right now it does a bit much

Separated into different methods. Currently, I always call this.updateScarbVersion() whenever the StatusBar is setup. Not sure if I should only call this when the config has the showInStatusBar value as true

---

vscode-cairo/src/statusBar.ts line 61 at r5 (raw file):

    } else {
      this.statusBarItem.hide();
    }

Done.

[mkaput]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @orizi, and @piotmag769)

[Arcticae]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marcuspang, @orizi, and @piotmag769)

---

vscode-cairo/src/statusBar.ts line 43 at r5 (raw file):

Previously, marcuspang (Marcus) wrote…

Separated into different methods. Currently, I always call this.updateScarbVersion() whenever the StatusBar is setup. Not sure if I should only call this when the config has the showInStatusBar value as true

I think it would be a good optimization but you can do it in a separate PR if you want :) It's not crucial

[mkaput]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marcuspang, @orizi, and @piotmag769)

[mkaput]

Thank you! ❤️❤️❤️❤️❤️