fix #106487 implement 'enablement' clause for viewsWelcome contribution's buttons

[gjsjohnmurray]

UPDATE: The actual solution reached during discussion in this PR can be seen in this comment

This PR fixes #106487 by adding an "activating" message as suggested by @joaomoreno

I included the "Learn More" button as a way of reserving the space that gets occupied by the "Clone Repository" button as soon as the git extension completes activation:

"Learn More" leads to the same place as the "read our docs" link in the second screenshot.

[gjsjohnmurray]

The SCM view now does the same:

[joaomoreno]

I'm not really sure the space reservation makes sense. Apart from the fact that the final message is taller than the loading one, making the button shift down, it doesn't feel like it's very useful to have a Learn More button there. I would simply remove it.

[gjsjohnmurray]

Here's how it looks when another extension is contributing as well:

And without the "Learn More" button:

Personally I think the placeholder button produces a less jarring experience, even though there's still some movement.

[gjsjohnmurray]

Another benefit of having a placeholder button is that I can add an animated codicon to its label. Currently it's not possible to use codicons in the plain text of welcome messages.

[joaomoreno]

After seeing it live, I am getting second thoughts about this.

How about we provide a way to declare a disabled button instead? And possibly have a tooltip which would describe why the button is disabled when hovered? That way we can have 100% the same contents thus causing no jumping at all and avoid having a strange Git extension is activating... message. What do you think?

[gjsjohnmurray]

I was just investigating that possibility. The viewsWelcome contribution mechanism almost lets me achieve this. There's an optional preconditions property to associate a context key expression with each button in the content. At the moment it's not wired up to get passed from an extension's viewsWelcome contribution, so I'll take a shot at doing that.

[joaomoreno]

Yeah it currently doesn't support preconditions, but we can certainly add it! It would be hard to add a tooltip for it though... since it would have to be a tooltip for when the precondition doesn't match.

[gjsjohnmurray]

How about this?

Doesn't do anything with a tooltip, but it'd be easy to add a static one that says something like "Clone a repository when the git extension has activated"

[joaomoreno]

Wait.. we already support it?! Or you just hacked it for now? YES looks much much better.

[gjsjohnmurray]

I've pushed the changes that make this work.

[joaomoreno]

As you may know, new API additions require an api labelled issue, arguing for the addition. Also, until finalized they should be protected by the proposed mechanism. Let me know if you need help with those tasks.

[joaomoreno]

Sorry @gjsjohnmurray, closing this as per #106487

Changes will still be here as a reference in case we decide to go back to it.

[gjsjohnmurray]

@joaomoreno ironically I was just blowing the dust of this and about to get back to you for help with steering the API change through the required process.

The only additions needed are in viewsWelcomeContribution.ts and viewsWelcomeExtensionPoint.ts, plus a bugfix in viewPaneContainer.ts. So having read in the Extension API Process doc the following:

All proposed APIs are defined in the vscode.proposed.d.ts file.

I'm not clear how to proceed. I don't need anything changed in vscode.d.ts or in its precursor vscode.proposed.d.ts

Please advise.

[joaomoreno]

I'm just wondering if this will really be worth the effort. I'm not entirely sold on the amount of work/extra code here vs. the added benefits.

[gjsjohnmurray]

I think it's worthwhile. The current experience of our extension's viewsWelcome contribution suddenly jumping down the screen because the Git extension has just completed its init is somewhat jarring. And the more extensions adopt viewsWelcome the more I foresee this kind of thing happening. The changes I propose (which really aren't extensive) provide extension developers a way of being better citizens in the shared space.

[joaomoreno]

@jrieken What do you think?

[gjsjohnmurray]

@jrieken What do you think?

In pictures, here's current behaviour, where my extension contributes text and a button which subsequently gets bumped down when the git extension finishes initializing:

And proposed, after the git extension has been tweaked to use the new capability:

(ignore the extra text in the contribution from my extension, which was added between capture of the "before" case and the "after" case)

[jrieken]

I like the feature/enhancement, but I think the proposed changed to the contribution need some polish, I'd suggest enablement instead of preconditions and a simple string instead of an array (like other context key expressions)

[joaomoreno]

Alright, let's make those changes here.

[gjsjohnmurray]

I like the feature/enhancement, but I think the proposed changed to the contribution need some polish, I'd suggest enablement instead of preconditions and a simple string instead of an array (like other context key expressions)

@jrieken fair point, but the advantage of the preconditions array is it better handles cases where a viewsWelcome contribution adds more than one button. For instance here the git extension contributes 2 buttons:

vscode/extensions/git/package.nls.json

Line 180 in ceeb974

"view.workbench.scm.empty": "In order to use git features, you can open a folder containing a git repository or clone from a URL.\n[Open Folder](command:vscode.openFolder)\n[Clone Repository](command:git.clone)\nTo learn more about how to use git and source control in VS Code [read our docs](https://aka.ms/vscode-scm).",

My preconditions proposal would operate like this:

      {
        "view": "scm",
        "contents": "%view.workbench.scm.empty%",
        "when": "config.git.enabled && workbenchState == empty",
        "preconditions": ["git.state == initialized", "git.state == initialized"],
        "group": "2_open@1"
      },

In this particular case it's right that both buttons enable/disable in lockstep, but there could be situations where each button needs a different clause.

If you're unconvinced then I'll implement enablement as you suggest and code things so the one enablement clause gets applied to every button in the contribution. That'll be OK for the Git contributions, and other multi-button contributions wanting to adopt the new feature will just have to live with the limitation.

[jrieken]

In this particular case it's right that both buttons enable/disable in lockstep, but there could be situations where each button needs a different clause.

I'd say that's wanted since this contribution point talks about about welcome views. Spreading information about enablement of commands inside it into the contribution seems too loose. So, I'd day that the enablement-properties applies to the whole welcome-view thing.

However, if we believe that fine grained enablement of individual command is needed then I would invest in a syntax that allows that to be expressed together with the command. That would keep information together and also not pose challenges about keeping the ordering correct - think about NLS and a translator reordering sentences/commands.

[gjsjohnmurray]

@jrieken thanks for your feedback. I have pushed the changes to make this work off an enablement string. I also updated the PR title and linked from the top of the description to the comment that shows before and after gifs.

@joaomoreno I think this is ready to merge.

[joaomoreno]

Thanks! 🍻