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

Views: Support contributable welcome view content #89080

Closed
joaomoreno opened this issue Jan 22, 2020 · 27 comments
Closed

Views: Support contributable welcome view content #89080

joaomoreno opened this issue Jan 22, 2020 · 27 comments

Comments

@joaomoreno
Copy link
Member

@joaomoreno joaomoreno commented Jan 22, 2020

Problem

The need to have meaningful and actionable content rendered on empty views has been arising across the workbench:

Proposal

Let's enable the following in the core:

  1. Add an overridable isEmpty(): boolean method to ViewPane which would let view panes dictate when they are empty, as well as onDidChangeEmpty event
  2. Create a core contribution point for empty view content, supporting very simple Markdown (links only)
  3. Enhance the simple Markdown to be able to detect isolated command links and render them as buttons
  4. Render all empty view contributions in a ViewPane when isEmpty() === true

We can then update our API by exposing an extension contribution point for the same core contribution point created above. Here's a couple of proposals:

contributes.documentation.VIEWID

"contributes": {
  "documentation": {
    "VIEWID": [{
       "label": "MARKDOWN",
       "when": "CONDITION"
    }]
  }
}

Alternatives for documentation: viewhelp, emptyview.

Alternatives for label: text, markdown, description.

contributes.documentation.view

"contributes": {
  "documentation": {
    "view": [{
       "label": "MARKDOWN",
       "when": "VIEWID && CONDITION"
    }]
  }
}

cc @sbatten for views
cc @isidorn for debug
cc @sandy081, @alexr00 for custom views
cc @bowdenk7 for scm
cc @jrieken for input on API conventions

@isidorn

This comment has been minimized.

Copy link
Contributor

@isidorn isidorn commented Jan 22, 2020

Looks good to me.

I prefer the contributes.documentation.VIEWID. That has the benefits of being more clear what it targets, looks more like the menu syntax and makes it easier for extensions to write when clauses.

I am personally not a big fan of the documentation name, however since we already use it in other places I think it makes sense to use it here also.

label is usually just a ux label, this feels to me as bit more. So I would prefer content or markdown.

Also note that debug would render something before this contributed section and something after this contributed section if possible. So the view should have the start, middle and end. With the middle being contributable.

fyi @weinand

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Jan 22, 2020

Add an overridable isEmpty(): boolean method to ViewPane which would let view panes dictate when they are empty

I guess we also need an event for when that's changing...

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Jan 22, 2020

Also adding @mjbvz for the doc contribution point

@mjbvz

This comment has been minimized.

Copy link
Contributor

@mjbvz mjbvz commented Jan 22, 2020

Could we put this on views contribution itself? Something like:

{
  "contributes": {
    "views": {
      "explorer": [
        {
          "id": "nodeDependencies",
          "name": "Node Dependencies",
          "when": "workspaceHasPackageJSON",
          "emptyContent": [{
            "markdown": "My empty message",
            "when": "CONDITION"
          }]
        }
      ]
    }
  }
}

If we prefer the documentation contribution point, I prefer the contributes.documentation.view approach as it lets us control the top level of the documentation namespace.

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Jan 23, 2020

One goal is that another extension can contribute getting-started text, e.g. a GH extension can contribute to the SCM view, etc. Not sure how that would look with this integrated approach

@isidorn

This comment has been minimized.

Copy link
Contributor

@isidorn isidorn commented Jan 23, 2020

Also it does not make a lot of sense for debug. Since all our extensions are not contributing views, and all of them want to contribute to our start view.

@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Jan 24, 2020

It turns out using markdown for this isn't feasible, since the library we currently use doesn't allow to prevent certain features (headers, tables, etc), not even using the renderer API. Since all we want are actually links, another option is to extract the work @bpasero did for notifications into something reusable.

@mjbvz

This comment has been minimized.

Copy link
Contributor

@mjbvz mjbvz commented Jan 24, 2020

@joaomoreno We use insane to sanitize rendered markdown

element.innerHTML = insane(renderedMarkdown, {

Could you configure it to only allow a very specific set of tags (try the allowedTags attribute)? Not sure if this would still be confusing for extension authors

@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Jan 27, 2020

We would only want to support links in this feature. No headers, no images, no styling, no lists, no tables. So, we'd need a markdown feature set filter.

@eamodio

This comment has been minimized.

Copy link
Member

@eamodio eamodio commented Feb 3, 2020

How would this behave if more than 1 extension registers an empty view content for the same view?

This also strikes me as quite similar to TreeView.message although it doesn't allow links (nor allowing other extensions to control). It would be nice if the content in this feature and TreeView.message would be the same, so that an extension could choose to provide its own empty content that can't be overridden.

Also, what would the signal for a TreeView to being empty -- no items at all? No items or message?

@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Feb 4, 2020

How would this behave if more than 1 extension registers an empty view content for the same view?

They pile up one on top of another.

This also strikes me as quite similar to TreeView.message although it doesn't allow links (nor allowing other extensions to control). It would be nice if the content in this feature and TreeView.message would be the same, so that an extension could choose to provide its own empty content that can't be overridden.

Yeah similar, though not the same, since we only want these messages appearing when a view is indeed empty, ie there's no other content to show.

Also, what would the signal for a TreeView to being empty -- no items at all? No items or message?

That's up to @alexr00 to define.

@alexr00

This comment has been minimized.

Copy link
Member

@alexr00 alexr00 commented Feb 4, 2020

I would show the empty content when there are no items and no message. That way the extension that contributes the view still has a way to show a message without all the empty content.

@eamodio

This comment has been minimized.

Copy link
Member

@eamodio eamodio commented Feb 4, 2020

When you say "pile up one on top of another" -- you mean last one wins or that they will all show and sort of stack vertically? Honestly neither of which feels like a good user experience to me.

@bowdenk7

This comment has been minimized.

Copy link
Member

@bowdenk7 bowdenk7 commented Feb 4, 2020

I think I would expect something like this where they stack vertically:

alt text

Then as soon as they open a folder where either git or svn contributes items, the empty one disappears. In this case, if the user clicks the git init button, I would expect something like:
alt text

@eamodio

This comment has been minimized.

Copy link
Member

@eamodio eamodio commented Feb 4, 2020

But what you are showing is 2 separate views (Git & SVN) each with one empty contribution. My question is what happens when there is more than 1 empty contribution for the same view.

@bowdenk7

This comment has been minimized.

Copy link
Member

@bowdenk7 bowdenk7 commented Feb 4, 2020

Ahh I see... What's an example of that?

@eamodio

This comment has been minimized.

Copy link
Member

@eamodio eamodio commented Feb 5, 2020

Say both GitLens and the GitHub PRs extension add an empty view for the SCM view.

@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Feb 5, 2020

One can't contribute an empty view. One can only contribute empty view contents.

Specifically for SCM, the viewlet is empty whenever there are no repositories registered, in which case it will render all empty view contents stacked on top of each other, ordered by a comparator of our choosing.

Other view domains will have to come up with their own concept of empty, such as custom views, which @alexr00 already addressed.

@eamodio

This comment has been minimized.

Copy link
Member

@eamodio eamodio commented Feb 5, 2020

Right -- I meant empty view contents. If this is for targeting "views", isn't the empty SCM viewlet a view container not a view itself?

Also will newlines be supported in the content (IMO it should be)? If so, having this in the package.json feels harder to manage, and also limiting in that you can't have any dynamic text.

@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Feb 6, 2020

SCM will behave in a special manner: when no repositories are registered, it will create an empty view. As soon as a repository comes along, that view gets removed.

Yes, newlines need to be supported. We'll go with package.json for now, and we can build from there as feature requests come in.

@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Feb 17, 2020

Converging on this proposal:

"contributes": {
  "viewsWelcome": [
    {
     "view": "view id",
     "contents": "here go the contents",
     "when": "when clause"
    }
  ]
}
@joaomoreno joaomoreno changed the title Views: Support contributable empty view content Views: Support contributable welcome view content Feb 17, 2020
@joaomoreno

This comment has been minimized.

Copy link
Member Author

@joaomoreno joaomoreno commented Feb 17, 2020

Closing this for now, since the following has landed in master:

  1. View welcome content registry
  2. Proposed View Welcome API contribution point
@isidorn

This comment has been minimized.

Copy link
Contributor

@isidorn isidorn commented Mar 6, 2020

Assigning to me, ropening and adding api-finalisation since we would like to finalise this API this milestone.

@alexr00

This comment has been minimized.

Copy link
Member

@alexr00 alexr00 commented Mar 6, 2020

Assigning to me too since this impacts custom views as well.

@alexr00 alexr00 self-assigned this Mar 6, 2020
@isidorn

This comment has been minimized.

Copy link
Contributor

@isidorn isidorn commented Mar 24, 2020

Feedback from Jonathan Carter
"Hey! Yep I tried it out, and it worked great. I'm looking forward to being able to use this for most of our team's extensions: Live Share, VS Online, etc."

@jrieken so we got some positive feedback on API from extensions. Let me know if there is something else we need to do before we finalise it.

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Mar 24, 2020

go for it. add a test plan item and these things

@isidorn isidorn closed this in 464ef8c Mar 24, 2020
@isidorn

This comment has been minimized.

Copy link
Contributor

@isidorn isidorn commented Mar 24, 2020

I have removed the proposed API check. Since this was already tested last milestone via #91269 I will simply reopen that test plan move it to this milestone and only let 1 person be assigned to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.