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

When changes are accepted/rejected the Uris are not notebook friendly #239555

Open
DonJayamanne opened this issue Feb 4, 2025 · 4 comments · May be fixed by #239556
Open

When changes are accepted/rejected the Uris are not notebook friendly #239555

DonJayamanne opened this issue Feb 4, 2025 · 4 comments · May be fixed by #239556
Assignees
Labels
chat notebook polish Cleanup and polish issue

Comments

@DonJayamanne
Copy link
Contributor

Today
Its difficult to tell what cells were updated
Image

Proposed
Image

@DonJayamanne DonJayamanne added chat notebook polish Cleanup and polish issue labels Feb 4, 2025
@DonJayamanne DonJayamanne self-assigned this Feb 4, 2025
@DonJayamanne DonJayamanne linked a pull request Feb 4, 2025 that will close this issue
@DonJayamanne
Copy link
Contributor Author

@bpasero I'd like to get a more user friendly url displayed in the chat panel for changes to notebook cells.
As you can see above, the changes that are accepted/rejected are not clearly defined as changes to cells.
The second screenshot is a proposed fix. The * Cell <number> is the standard display format for cell uris.

I'm not entirely sure whether making the change to the LabelService as in the PR is the right approach.
I've added a flag as I'm not entirely sure where this is used.
That being said, any Cell URI should ideally be displayed in the above mentioned format, thereby making the flag unnecessary.
Then again, not sure this is the right solution.

Please advice on how we can improve the display format.
Today the formatting is done in ResourceLabelWidget in https://github.com/microsoft/vscode/blob/influential-koala/src/vs/workbench/browser/labels.ts#L484

@bpasero
Copy link
Member

bpasero commented Feb 4, 2025

Are these specific URIs for cells? Then why not register a handler for the formatting of these?

registerFormatter(formatter: ResourceLabelFormatter): IDisposable {

@DonJayamanne
Copy link
Contributor Author

Are these specific URIs for cells?

Yes

Thanks I should have mentioned that I did look into that & unfortunately that formatter doesn't support setting prefixes to the Uri dynamically based on whether this is the first or second or third cell. & unfortunately that information isn't part of the Uri.

The only way I can do this is to register a formatter just before i invoke getUriLable for a specific Uri with the predefined suffix in the label e.g. * Cell 2 or the like. & then remove that formatter immediately after that.

I don't think thats right, after all I might as well manually add the suffix * Cell 2 to the label (string) returned by getUriLabel

@bpasero
Copy link
Member

bpasero commented Feb 5, 2025

It smells like debt to me to encode notebook specific logic into a core service like labels, so I would not go down that path. Can you address this for now just in chat?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat notebook polish Cleanup and polish issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants