`PhantomSet` accepting a `View` feels like a mistake

[rchl]

Description of the bug

PhantomSet accepts a View as an argument but phantoms are internally bound to a Buffer and not a View.

Why is that a problem? Because Split (Clone) views.

• if one creates a PhantomSet from a ViewListener and splits a view then the split view will also show the phantom (as expected, I would say) but closing the primary view will ~~remove phantoms from the clone (unexpected)~~ break the phantoms (no long possible to update the PhantomSet).
• If one overrides applies_to_primary_view_only to return False in ViewListener then Split view will also create PhantomSet and there will be duplicate phantoms.

If I want the phantoms to persist closing of the primary view then I need to track when primary view is closed and create new PhantomSet with one of the remaining views passed to the constructor which is messy.

Note that the normal regions added with add_regions are bound to the View and not Region so have to be added separately for each clone. Same for annotations (which are technically also regions). This in theory could also apply to phantoms and could be seen as more consistent but I imagine that for performance and memory reasons binding to Buffer is likely a preferred option.

It seems to me like regions should take Buffer as an argument and not a View.

While I don't expect this to be changed in the future due to it being a breaking change, I suppose the API could be changed to accept either View or Buffer.

Steps to reproduce

Not so much a reproduction but a piece of code to play with

from sublime import LAYOUT_INLINE, Phantom, PhantomSet, Region
import sublime_plugin


class ViewEventListener(sublime_plugin.ViewEventListener):

    # @classmethod
    # def applies_to_primary_view_only(cls) -> bool:
    #     return False

    def on_activated_async(self) -> None:
        print('Activated {}. primary?: {}'.format(self.view, self.view.is_primary()))
        self.phantoms = PhantomSet(self.view)
        self.phantoms.update([Phantom(Region(0, 10), '!!!!!!', layout=LAYOUT_INLINE)])

Expected behavior

Better API

Actual behavior

Kinda broken API

Sublime Text build number

4160

Operating system & version

macOS

(Linux) Desktop environment and/or window manager

No response

Additional information

No response

OpenGL context information

No response

[kaste]

While the API might be confusing, isn't

but closing the primary view will remove phantoms from the clone

just an ordinary bug?

However, because of the API mistake I have

phantoms_per_buffer = {}  # type: Dict[sublime.BufferId, sublime.PhantomSet]
def get_phantom_set(view):
    # type: (sublime.View) -> sublime.PhantomSet
    bid = view.buffer_id()
    try:
        return phantoms_per_buffer[bid]
    except LookupError:
        rv = phantoms_per_buffer[bid] = sublime.PhantomSet(view, "SLInlineHighlighter")
        return rv

in SublimeLinter and elsewhere as an indirection.

[rchl]

While the API might be confusing, isn't

but closing the primary view will remove phantoms from the clone

just an ordinary bug?

I suppose it could take a View but internally automatically get the Buffer from that and only rely on that.

I still find it weird (to put it mildly) that it takes View if it really applies at the Buffer level. If it'd really worked on the View level then it should need to be set separately per View.

[rchl]

Sorry, I was wrong about one detail. The phantoms don't go away on closing the primary view.

from sublime import LAYOUT_INLINE, Phantom, PhantomSet, Region
import sublime_plugin


phantoms = None

class ViewEventListener(sublime_plugin.ViewEventListener):

    def on_activated_async(self) -> None:
        global phantoms
        print('Activated {}. primary?: {}'.format(self.view, self.view.is_primary()))
        phantoms = PhantomSet(self.view)
        phantoms.update([Phantom(Region(0, 10), 'PHANTOM', layout=LAYOUT_INLINE)])

What that leaves this issue with? I think maybe just the confusing part about it taking a View?

[rchl]

I was wrong about being wrong. The phantoms might not go away after closing the primary view but they become "dead" instead - updating PhantomSet doesn't update the view anymore.

This will update the phantom text every second. After creating a split view and closing the primary view, it stops updating even though the timeout is still running.

phantoms = None

class ViewEventListener(sublime_plugin.ViewEventListener):

    def __init__(self, view: sublime.View) -> None:
        global phantoms
        super().__init__(view)
        self.phantom_text = '[PHANTOM]!'
        phantoms = PhantomSet(self.view)
        self.update_phantom()

    def update_phantom(self) -> None:
        global phantoms
        if not phantoms:
            print('no global phantoms set')
            return
        print('updating phantoms')
        self.phantom_text += '!'
        phantoms.update([Phantom(Region(4, 10), self.phantom_text, layout=LAYOUT_INLINE)])
        sublime.set_timeout(self.update_phantom, 1000)

[rchl]

@kaste I think your indirection will also fail when the primary view is closed - the PhantomSet won't react to updates anymore.

[kaste]

Yes the indirection totally fails but you found or made a good description of a bug not just an API mistake. You describe a bug here I never totally get hold off. If you have phantoms for example in SublimeLinter and a cloned/duplicated view they still work if you close the clone, but if you have a duplicated view and close the first ("primary") view the phantoms just stay and won't react anymore.

[kaste]

@rchl I think this is clearly a bug. I think "feels like a mistake" is too vague, you might want to change the title. No?

[rchl]

Well, depends what one considers to be the issue here...

One could argue that the fact that PhantomSet takes a View but makes phantoms also show up on clones (which are not the same Views) is the issue. If having separate PhantomSets per view is the intended behavior then the issue with phantoms stopping working after closing the primary view is moot.

If the intention is that PhantomSet applies to all clone Views then I would say that it should take a Buffer as an argument. And of course not break on closing the primary View but if it's not bound to specific View then it should have no problems working after closing the primary view.

I'll leave the issue title as is and let the ST devs decide the best course of action here.

[FichteFoll]

One could argue that the fact that PhantomSet takes a View but makes phantoms also show up on clones (which are not the same Views) is the issue.

I support this interpretation as it was the first thought that came to my mind after reading the issue summary and would also be consistent with added regions. (And fixing it doesn't require a breaking API change like taking a buffer would.)