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

PhantomSet.update does not always remove all phantoms #2897

Open
braver opened this issue Jul 23, 2019 · 11 comments

Comments

@braver
Copy link

commented Jul 23, 2019

Description

In some situations phantoms no longer present in the PhantomSet after an update(), are not removed from the view.

Steps to reproduce

For some reason this is easy triggered when using the ColorHints plugin and editing multiple colors on the same line. It also seems that undoing and redoing changes in rapid fashion frequently causes "dangling" phantoms. The ColorHints plugin builds a list of phantoms that should be displayed, and passes that on to PhantomSet.update(). According to the documentation, any phantoms not in that list should be removed.

In this image on the lef you can see the list being printed, and currently it's empty. On the right you can see that 4 phantoms are still visually present in the view.

Screen Shot 2019-07-23 at 20 44 51

Expected behavior

PhantomSet.update() should completely remove all phantoms that are not in the list passed to it.

Actual behavior

Some region "decorations" like phantoms, but the similar behavior has also been noticed in highlights like the ones produced by SublimeLinter: undoing changes sometimes makes highlights visually re-appear, but they're no longer attached to anything and cannot be removed.

Environment

  • Build: 3208
  • Operating system and version: macOS 10.14.5
@jskinner

This comment has been minimized.

Copy link

commented Jul 24, 2019

It'd be great if you could also print the contents of phantom_set.phantoms

@jfcherng

This comment has been minimized.

Copy link

commented Jul 24, 2019

@jskinner I have probably another case which I am able to stably and easily reproduce.

My phantom_set.phantoms is only a phantom object but there are multiple phantoms on the screen.

錄製_2019_07_24_11_40_58_343

The plugin is https://github.com/jfcherng/open_in_browser.

Steps to reproduce as shown in the above gif.

  • Copy the URL (important: with a leading new line)
  • Repeatedly preform paste (ctrl+v) and undo (ctrl+z)
  • BOOM!!
@jskinner

This comment has been minimized.

Copy link

commented Jul 24, 2019

Thanks for the repro, this will be fixed in the next build

The underlying phantom API is fine, the bug is in PhantomSet: it stores the known phantoms in a dict, but changes the value returned by __hash__ while the phantom objects are still in the dict, resulting in them being forgotten.

@jskinner

This comment has been minimized.

Copy link

commented Jul 24, 2019

As a workaround, you can embed the fixed version of PhantomSet directly in your plugin:

EDIT: Deleted incompatible code

@braver

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

Thanks!

@jfcherng

This comment has been minimized.

Copy link

commented Jul 24, 2019

@jskinner

    new_phantoms = set(new_phantoms)

This line seems to be problematic. It's causing TypeError: unhashable type: 'Phantom' on my side.

I tried set([sublime.Phantom(sublime.Region(0), 'whatever', sublime.LAYOUT_INLINE)]) in the console and get the same exception. Phantom has no __hash__() method.


I tried some __hash__() implementations for Phantom.

The following doesn't fix the original bug.

    def __hash__(self):
        return hash(';'.join(map(str, [
            self.region,
            self.content,
            self.layout,
            hash(self.on_navigate),
        ])))

The following fixes the original bug but it just re-renders everything which causes unwanted performance/visual issues.

    def __hash__(self):
        return hash(self.id)
@FichteFoll

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

This may also be the reason for other relatd weird behavior, such as https://forum.sublimetext.com/t/dev-build-3118/21270/49?u=fichtefoll (unfortunately the webm is missing, but here's a reupload).

@jskinner

This comment has been minimized.

Copy link

commented Jul 25, 2019

Sorry, I forgot that there have been other changes to sublime_plugin.py in our git repo, including Phantom and Region implementing __hash__. The above code won't work without that.

@jskinner

This comment has been minimized.

Copy link

commented Jul 25, 2019

A proper workaround is below, which is derived from the implementation of PhantomSet in the most recent Sublime Text build:

class PhantomSet(object):
    def __init__(self, view, key=""):
        self.view = view
        self.key = key
        self.phantoms = []

    def __del__(self):
        for p in self.phantoms:
            self.view.erase_phantom_by_id(p.id)

    def update(self, new_phantoms):
        # Update the list of phantoms that exist in the text buffer with their
        # current location
        regions = self.view.query_phantoms([p.id for p in self.phantoms])
        for i in range(len(regions)):
            self.phantoms[i].region = regions[i]

        for p in new_phantoms:
            try:
                # Phantom already exists, copy the id from the current one
                idx = self.phantoms.index(p)
                p.id = self.phantoms[idx].id
            except ValueError:
                p.id = self.view.add_phantom(
                    self.key, p.region, p.content, p.layout, p.on_navigate)

        new_phantom_ids = set([p.id for p in new_phantoms])
        for p in self.phantoms:
            # if the region is -1, then it's already been deleted, no need to
            # call erase
            if p.id not in new_phantom_ids and p.region != Region(-1):
                self.view.erase_phantom_by_id(p.id)

        self.phantoms = new_phantoms
@jfcherng

This comment has been minimized.

Copy link

commented Jul 25, 2019

@jskinner Thanks! It works, at least in my case.

@FichteFoll

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

(Also just FYI, I couldn't reproduce the issue I referenced even with the current sublime.py.)

@FichteFoll FichteFoll added this to the Build 3209 milestone Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.