Fix on_sheet_opened called twice from uri_handler by rchl · Pull Request #2914 · sublimelsp/LSP

rchl · 2026-05-17T21:27:41Z

This is a bit rushed fix so might need to have another, more thorough, look.

Problem:

When following the migration guide and using session.open_scratch_buffer, the _on_sheet_opened was called two times. Once with the uri provided by the handler and then overridden with the original uri.

Remove the _on_sheet_opened call chained to the handler - if handler uses session.open_scratch_buffer then it is not needed. When handler doesn't use session.open_scratch_buffer then this might be problematic to not call it though...

(Another issue is that when following the migration guide the types are not validating because session.open_scratch_buffer returns View while uri_handler wants sheet so there is extra then needed to convert the return value.)

jwortmann · 2026-05-17T22:15:24Z

Isn't the right solution to just remove it from open_scratch_buffer at

LSP/plugin/core/sessions.py

Line 1572 in 9c689cc

self._on_sheet_opened(view.sheet(), uri, r).then(resolve)

Then it would get called a single time in all cases, no?

rchl · 2026-05-18T06:17:06Z

Isn't the right solution to just remove it from open_scratch_buffer at

LSP/plugin/core/sessions.py

Line 1572 in 9c689cc

self._on_sheet_opened(view.sheet(), uri, r).then(resolve)

Then it would get called a single time in all cases, no?

No, due to fact that then this code will call _on_sheet_opened with the original uri and r which might not be the same as the ones that the uri_handler passed to open_scratch_buffer.

The example case is: uri handler gets an URI with a fragment. The fragment declares position in the file so it's meant to be stripped from the uri before its set as lsp_uri. This code then overwrites it to be the full uri with the fragment.

But then I also think it's a bit rushed fix and ideally the _on_sheet_opened would always get called after the handler. But then there would need to be be some way of telling whether it was already called.

rchl · 2026-05-18T06:57:17Z

But then I also think it's a bit rushed fix and ideally the _on_sheet_opened would always get called after the handler. But then there would need to be be some way of telling whether it was already called.

But perhaps that's not that important because if you are opening a View then likely you want to use session.open_scratch_buffer. If not a View then _on_sheet_opened is not relevant since it only works for Views.

jwortmann · 2026-05-18T18:13:12Z

I think at

LSP/plugin/core/sessions.py

Line 1465 in 828a94d

    
           return handler(uri, flags).then(lambda sheet: self._on_sheet_opened(sheet, uri, r))

we should just remove the fragment from the uri that is passed into _on_sheet_opened. Possibly we might also want to normalize the URI at

LSP/plugin/core/sessions.py

Line 1567 in 828a94d

    
           view.settings().set('lsp_uri', uri)  # Preserve original URI given by the language server

but I think that shouldn't really be relevant for URIs with custom schemes (or perhaps it would even be bad if we send back slightly different URIs to the server, so probably normalizing wouldn't be a good idea and is only relevant for file: URIs).

And I think using the original r is still the correct thing, because it might come from a Location and then that would/should take precedence over some row/column that might be derived from the URI fragment. If r is None, the position in the view can still be controlled from the custom URI handler.

jwortmann · 2026-05-18T18:29:11Z

Actually setting lsp_uri to the view returned from the handler might not always be good; for example LSP-Tinymist uses some really weird URIs to open other files either in the editor or in an external viewer (iirc the only place where I have seen these URIs are when hovering over links to image files).

https://github.com/sublimelsp/LSP-Tinymist/blob/5173ee56e777e3587f4823ef82107905e7445cd5/plugin.py#L247-L260

rchl · 2026-05-18T18:34:26Z

I think at

LSP/plugin/core/sessions.py

Line 1465 in 828a94d

return handler(uri, flags).then(lambda sheet: self._on_sheet_opened(sheet, uri, r))

we should just remove the fragment from the uri that is passed into _on_sheet_opened.

That would work for my use case.

LSP-yaml supports inline hints that when clicked open schema for the corresponding yaml file. In that case it triggers opening of URI like:

json-schema://www.schemastore.org/github-workflow.json#https%3A%2F%2Fwww.schemastore.org%2Fgithub-workflow.json

where the fragment is the URL to download the schema from.

In that case we want to remove the fragment and use json-schema://www.schemastore.org/github-workflow.json URI for the opened file. Then document links within this file will look like:

json-schema://www.schemastore.org/github-workflow.json#412,14

and LSP will handle those automatically to support jumping to specified location.

But if that wouldn't work for LSP-Tinymist then we should just allow setting URI manually which this change allows.

jwortmann · 2026-05-18T19:01:52Z

In the Tinymist use case the server should better just use regular file: URIs, and since it seems to be only used for image files, allowing to set lsp_uri manually isn't really needed for that use case.

I think I would prefer setting the URI without fragment from _on_sheet_opened. That would ensure that the URI is set even when not using Session.open_scratch_buffer.

rchl · 2026-05-18T19:21:38Z

I think I would prefer setting the URI without fragment from _on_sheet_opened. That would ensure that the URI is set even when not using Session.open_scratch_buffer.

But then there is the second issue of it being called twice if handler uses session.open_scratch_buffer. Which shouldn't be a big issue in practice but still weird.

Or if we remove _on_sheet_opened from session.open_scratch_buffer then it will be less useful if someone wants to use it from somewhere else.

This reverts commit 5d89825.

jwortmann · 2026-05-18T19:28:49Z

Or if we remove _on_sheet_opened from session.open_scratch_buffer then it will be less useful if someone wants to use it from somewhere else.

The purpose for _on_sheet_opened is to be used when a URI with custom scheme gets opened. So I'd say that's fine and I would not expect to call that when Session.open_scratch_buffer is used from somewhere else. Random scratch buffers will automatically get the buffer URI from the ViewListener.

netlify · 2026-05-18T19:29:09Z

✅ Deploy Preview for sublime-lsp ready!

Name	Link
🔨 Latest commit	`dbab2e0`
🔍 Latest deploy log	https://app.netlify.com/projects/sublime-lsp/deploys/6a0c986ce57f2f00089b9520
😎 Deploy Preview	https://deploy-preview-2914--sublime-lsp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

rchl · 2026-05-18T19:33:46Z

Or if we remove _on_sheet_opened from session.open_scratch_buffer then it will be less useful if someone wants to use it from somewhere else.

The purpose for _on_sheet_opened is to be used when a URI with custom scheme gets opened. So I'd say that's fine and I would not expect to call that when Session.open_scratch_buffer is used from somewhere else. It will automatically get the buffer URI from the ViewListener.

Not always, LSP-json wants to handle custom json-schema scheme files opened by LSP-yaml:
https://github.com/sublimelsp/LSP-json/blob/2efca03a275a4b6c1e3ee5dd657f445276c1d76f/LSP-json.sublime-settings#L11-L12

That said, I've realized that just returning Promise(None) from the handler will avoid calling _on_sheet_opened again which is good enough solution for me.

We could try to make other discussed changes but not sure if needed and that we've arrived at perfect solution.

jwortmann · 2026-05-18T19:37:13Z

I would even modify open_scratch_buffer such that title must be provided (not None), remove uri argument, and return type would be Promise[sublime.View].

Then the example in the URI handler for the docs would be

return session.open_scratch_buffer(...).then(lambda view: view.sheet())

jwortmann · 2026-05-18T19:45:49Z

Still doesn't feel optimal to me. I would expect open_scratch_buffer to be possible to call without proving the URI, and then URI default to the buffer:123 URIs that we use.

rchl · 2026-05-18T19:47:00Z

I would even modify open_scratch_buffer such that title must be provided (not None), remove uri argument, and return type would be Promise[sublime.View].

Then the example in the URI handler for the docs would be
return session.open_scratch_buffer(...).then(lambda view: view.sheet())

Made title required and changed return type.
But I'm still hesitant to remove uri and _on_sheet_opened from it because:
a) there might be use cases that want the lsp_uri to be something more custom.
b) session.open_scratch_buffer will be less flexible if someone wants to use it from some other place

jwortmann · 2026-05-19T09:42:36Z

To be honest, I still don't like it. We would also need to update the docstring: "Return a Promise resolved with the opened sheet if you opened the URI as an editor tab. Unless you used this particular function, then return Promise[None] for some internal reasons that we don't tell you here ¯\(ツ)/¯"

The reason for having the sheet/view returned is that the function that called Session.open_uri_async might further need to work with the returned view. For example for changes in WorkspaceEdit to a document with custom URI scheme.

rchl · 2026-05-19T17:08:44Z

    ) -> Promise[sublime.View | None] | None:
        # I cannot type-hint an unpacked tuple
-        pair: PackagedTask[tuple[str | None, str, str]] = Promise.packaged_task()
+        pair: PackagedTask[tuple[str, str, str]] = Promise.packaged_task()
+        promise, resolve = pair
        # It'd be nice to have automatic tuple unpacking continuations
-        callback = lambda a, b, c: pair[1]((a, b, c))  # noqa: E731
+        callback = lambda a, b, c: resolve((a or 'untitled', b, c))  # noqa: E731
        if plugin.on_open_uri_async(uri, callback):
-            result: PackagedTask[sublime.View | None] = Promise.packaged_task()
-            pair[0].then(lambda tup: self.open_scratch_buffer(*tup, uri, r, flags, group)).then(result[1])
-            return result[0]
+            return promise.then(lambda tup: self.open_scratch_buffer(*tup, flags, group)) \
+                .then(lambda view: self._on_sheet_opened(view.sheet(), uri, r))
        return None


Two PackagedTask's were unnecessary - one is enough.

Fix on_sheet_opened called twice from uri_handler

5d89825

rchl added 2 commits May 18, 2026 21:24

Revert "Fix on_sheet_opened called twice from uri_handler"

afb3978

This reverts commit 5d89825.

update docs

d415e8c

remove fragment from lsp_uri

85043ec

rchl commented May 18, 2026

View reviewed changes

Comment thread plugin/core/sessions.py

require title

d4a4b60

rchl commented May 18, 2026

View reviewed changes

Comment thread plugin/core/sessions.py Outdated

rchl added 3 commits May 18, 2026 21:57

_on_sheet_opened doesn't need to use promise

d5551a2

make uri optional

0bee883

Merge branch 'main' into fix/sheet-return

89e1087

jwortmann reviewed May 18, 2026

View reviewed changes

Comment thread plugin/core/sessions.py Outdated

untitled

f8dac33

address

dbab2e0

rchl commented May 19, 2026

View reviewed changes

resolve

30492ae

jwortmann approved these changes May 19, 2026

View reviewed changes

rchl merged commit 01dbb02 into main May 19, 2026
8 checks passed

rchl deleted the fix/sheet-return branch May 19, 2026 20:22

Conversation

rchl commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwortmann commented May 17, 2026

Uh oh!

rchl commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchl commented May 18, 2026

Uh oh!

jwortmann commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwortmann commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchl commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwortmann commented May 18, 2026

Uh oh!

rchl commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwortmann commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for sublime-lsp ready!

Uh oh!

rchl commented May 18, 2026

Uh oh!

jwortmann commented May 18, 2026

Uh oh!

Uh oh!

jwortmann commented May 18, 2026

Uh oh!

rchl commented May 18, 2026

Uh oh!

Uh oh!

Uh oh!

jwortmann commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchl May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rchl commented May 17, 2026 •

edited

Loading

rchl commented May 18, 2026 •

edited

Loading

jwortmann commented May 18, 2026 •

edited

Loading

jwortmann commented May 18, 2026 •

edited

Loading

rchl commented May 18, 2026 •

edited

Loading

rchl commented May 18, 2026 •

edited

Loading

jwortmann commented May 18, 2026 •

edited

Loading

netlify Bot commented May 18, 2026 •

edited

Loading

jwortmann commented May 19, 2026 •

edited

Loading