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

resourceLabelFormatters support more variables like ${query.*} #68201

Closed
eamodio opened this issue Feb 8, 2019 · 18 comments
Closed

resourceLabelFormatters support more variables like ${query.*} #68201

eamodio opened this issue Feb 8, 2019 · 18 comments
Assignees
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@eamodio
Copy link
Contributor

eamodio commented Feb 8, 2019

I've been playing with the new resourceLabelFormatters extension point and I'm not really sure I'm getting it fully.

First, it doesn't seem like it applies everywhere in the UI -- for example, the tooltips on tabs don't seem to be affected.

Also what is workspaceSuffix used for? I can't seem to see any change when using it. Also is there any more formatting possible with label or just the url tokens?

Any help/clarification would be great -- thanks!

/cc @isidorn

@eamodio
Copy link
Contributor Author

eamodio commented Feb 9, 2019

I would also really like to be able strip the leading / from the ${path} and also maybe have an OS-dependent separator (or set separator to undefined or null to indicate to use the right one for the OS)

@isidorn
Copy link
Contributor

isidorn commented Feb 18, 2019

@eamodio the formatters should be applied everywhere in the UX. If not we need to adopt it in the missing places. @bpasero for using the label service in the tabs tooltips.
@eamodio are there other places where the formatting is not respected?

workspaceSuffix should be visible in the explorer title area for example. Do you see it there?
Stripping the leading / from the ${path} is currently not supported, also indicating an OS seperator.
Though we could add this.
Do you have more feedback regarding this feture?

@isidorn isidorn added this to the March 2019 milestone Feb 18, 2019
@bpasero
Copy link
Member

bpasero commented Feb 18, 2019

@isidorn I would assume this possibly should work already because I end up calling EditorInput.getDescription(LONG) which seems to be this.labelService.getUriLabel(this.resource)

@isidorn
Copy link
Contributor

isidorn commented Feb 18, 2019

It seems like you are doing the right thing

@bpasero
Copy link
Member

bpasero commented Feb 18, 2019

🏆

@eamodio
Copy link
Contributor Author

eamodio commented Feb 18, 2019

The tooltip on the tabs has been fixed since I reported it -- thanks! As for the workspaceSuffix I'm definitely missing something. Since AFAIK you can't open a folder that isn't a real on disk folder, the title that shows up on the explorer title area is the name of the workspace itself, and inside the explorer view where the name of the folder is shown the suffix doesn't show up.

IMO stripping the leading / would be a great option as would having an OS dependent separator:
image

Supporting more advance matching would be great. For example in GitLens (see the above screenshot) -- I pack the git ref into the authority of the Uri, which is challenging when the ref is a branch name (since branches can contain / which I have to swap out for a unicode character that looks similar to / so it will be displayed -- just encoding it didn't work). If I could avoid packing anything in the authority it would be helpful. I also pack a json object into the query string (similar to the built-in Git extension) which has many properties with details about the uri reference

See the code here:
https://github.com/eamodio/vscode-gitlens/blob/15d57bc797973f95c167784d182e1b27dc8e9a09/src/git/gitUri.ts#L400-L406

So it would be wonderful if there was a variable syntax that could pull data out of the query string (and assume it is an object) -- e.g. ${qs.<property-name>} and if it failed (or returns undefined/null) for any reason it would just get coerced into an empty string. Or could go further and support something like ${qs.<property-name>|<missing-value>} where any failure/falsy would use the <missing-value> instead -- but I don't think that is necessarily needed (since an ext can always add a different prop to the qs object for that if needed).

That would allow me to get rid of the strange authority packing and hacks, and just use the querystring directly as below:

        "resourceLabelFormatters": [
            {
                "scheme": "gitlens",
                "authority": "*",
                "formatting": {
                    "label": "${path} (${qs.ref})",
                    "separator": "/"
                }
            }
        ],

@isidorn Thoughts?

@isidorn
Copy link
Contributor

isidorn commented Feb 19, 2019

@eamodio Thanks a lot for feedback, however I just came back from vacation and am catching up with a bunch of issues. Will be able to only get back to you next milestone probably.

@isidorn
Copy link
Contributor

isidorn commented Mar 15, 2019

@eamodio ok I have processed your feedback.
You want that the resource label service supports more variables.
#{qs.ref} for query string.

Due to that I will rename this issue and mark it as a feature request. Though this is not on our immediate plan I think it makes sense overall.

@isidorn isidorn changed the title resourceLabelFormatters doesn't apply everywhere resourceLabelFormatters support more variables like ${querryString.} Mar 15, 2019
@isidorn isidorn added the feature-request Request for new features or functionality label Mar 15, 2019
@isidorn isidorn modified the milestones: March 2019, Backlog Mar 15, 2019
@eamodio
Copy link
Contributor Author

eamodio commented Mar 15, 2019

@isidorn would a PR for this be accepted?

@isidorn
Copy link
Contributor

isidorn commented Mar 18, 2019

eamodio added a commit to eamodio/vscode that referenced this issue Jul 23, 2019
@isidorn isidorn modified the milestones: Backlog, July 2019 Jul 30, 2019
@isidorn isidorn added the verification-needed Verification of issue is requested label Jul 30, 2019
@isidorn
Copy link
Contributor

isidorn commented Jul 30, 2019

Fixed via #71874

@isidorn isidorn closed this as completed Jul 30, 2019
@jrieken
Copy link
Member

jrieken commented Jul 30, 2019

@isidorn What's to verify here? Is this new (soft) API? Did you settle on qs, iff so why not query as all other names following URI naming?

@isidorn
Copy link
Contributor

isidorn commented Jul 30, 2019

Yes this is kind of new soft proposed API.
Currently we only supported scheme authority and path. And this is adding qs.
And yes query might be a better name here.
To verify: code review + current unit tests should be fine.
Since in practice I believe only GitLens is using this and @eamodio already veriified it works.

@jrieken
Copy link
Member

jrieken commented Jul 30, 2019

Ok, reopening then to change the name to query.

@jrieken jrieken reopened this Jul 30, 2019
@eamodio
Copy link
Contributor Author

eamodio commented Jul 30, 2019

@jrieken do you want me to send another PR? Or can I just push the changes to the same PR (even though its been merged)?

@jrieken
Copy link
Member

jrieken commented Jul 30, 2019

I'd say, better a new one

@eamodio
Copy link
Contributor Author

eamodio commented Jul 30, 2019

Incoming shortly...

@isidorn
Copy link
Contributor

isidorn commented Jul 30, 2019

Adderessed via #78190

@isidorn isidorn closed this as completed Jul 30, 2019
isidorn added a commit that referenced this issue Jul 30, 2019
@jrieken jrieken added the verified Verification succeeded label Jul 30, 2019
@eamodio eamodio changed the title resourceLabelFormatters support more variables like ${querryString.} resourceLabelFormatters support more variables like ${query.*} Aug 8, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants