On-gutter-hover popups conflict

[braver]

We ran into this when developing a popup for SublimeLinter. When hovering the gutter, it should show a popup with all linter errors for a given line. However, in combination with GitGutter, the GitGutter popup tends to show instead. It kinda feels like it’s a race condition that decides which shows.

There is this protected region setting, and that does help deciding which icon shows in the gutter. And that works. Perhaps it should also be used to decide if a popup should be shown. As far as I can tell, protected regions aren’t checked for popups. Which is good for bookmarks etc.

So I don’t know what a good approach would be to improve the interop between these two plugins, but I do want to put this out there in hope someone has a brilliant idea ☺️

[deathaxe]

Basically I can see three options:

1. Use the existing protected_regions
2. Add a new popup_protected_regions to disable diff popup for regions known to provide higher priority popup functionality.
3. Show diff popup only for lines occupied by a git_gutter_... region.

Option 1. and 3. should result in equal behavior while 3. is faster and easier to check as all git_gutter_... regions start at the beginning of a line so their beginning boundary is already equal to the point argument of the on_hover handler. It would further result in a consistent behavior as diff popup might not be expected if no gutter mark exists. But as you already stated, it would disable diff popup even though there is no other popup to show which is true for bookmarks and maybe others.

Option 2 would add more flexibility to decide when to show the diff popup. With a little bit of help from foreign packages it could even be much faster than option 1 or 3. What I have in mind is each popup (e.g. SublimeLinter) adding an invisible sublime_linter.popup_region if it shows a popup, with the point from on_hover. The popup would need to implement the on_hide event to remove the region again.

Example:

import sublime
import sublime_plugin

class EventListener(sublime_plugin.EventListener):

    def on_hover(self, view, point, hover_zone):

        failed = False

        if hover_zone != sublime.HOVER_GUTTER:
            return

        def remove_region():
            """Remove popup region, if popup gets hidden."""
            view.erase_regions("package_name.popup_region")

        def on_async_done():
            """An async task is completed with the popup being finally added."""
            if failed:
                return remove_region()

            view.show_popup(content="Hello World",
                            location=point,
                            on_hide=remove_region)

        # simulate async task
        sublime.set_timeout_async(on_async_done, 200)

        # mark line as occupied by "package_name"
        view.add_regions("package_name.popup_region",
                        regions=sublime.Region(point),
                        flags=sublime.HIDDEN)

The package_name.popup_region could then be checked by GitGutter and maybe vice versa.

[braver]

Yeah, I think 1 and 3 both have the downside that it might block a popup, but the upside that it's predictable what popup to expect. I think in that sense it's elegant, since the popup is like an explanation of the gutter mark. But it also means I may not have access to the gitgutter popup and a quick "reset" if I made a lint error on a line.

One gutter isn't enough... ;)

[deathaxe]

I may not have access to the gitgutter popup and a quick "reset"

You always have: ctrl+shift+alt+z

One gutter isn't enough...

The "line changed" gutter is something special and would be worth a dedicated area like in VS Code or Atom just right next to the line numbers. I could even imagine to use GitGutter to track normal files not part of a repo ;-) by just using a copy of the file when opened to do comparison.

[braver]

You always have: ctrl+shift+alt+z

Oh yeah ☺️

Every editor has a vcs gutter thing these days. I won’t work without one anymore. You think we can badger Will into doing something about it?

I still have halve a mind to remove the gutter popup from SL though, so we can just let the issue simmer for a while.

[r-stein]

If this is not really an issue with other packages we can just use #477

[FichteFoll]

GitGutter and linters (i.e. SublimeLinter) seem like the most obvious contenders for gutter popups. Others that come to mind would be debuggers, but I haven't used any of those yet and have no idea about their usage of popups. It might come up in the future, however.

Regarding a general solution, maybe a User setting could be introduced that will be shared by all contending packages. On first launch, they would insert their "region" or "key" in a list at some position, e.g. before or after a certain potential other key. Then it needs to look for a potential conflict with any of these higher in that list, similar to #477, before adding its own popup.

Or, you know, just make a dependency that does all that. Make it provide a useful API like "should I render a popup here?" and have it store the default precedence list. Removes the need for all the duplicated code. Could take care of gutter icons as well, although that will be harder to synchronize.

[braver]

Those are good points. Now we’re tweaking GitGutter to deal with other packages that also provide gutter marks, but LSP also creates marks, and there are dedicated linters out there too and you can use all of them together. So we could do with better gutter management all around.

[deathaxe]

Hmm. Reading @FichteFoll's suggestion triggered the following idea:

How about a dependency, which itself implements an EventListener that handles the on_hover(). It would than call an on_request_popup() method of all registered view_event_listeners which define it. This method returns True synchronously if a plugin attempts to show a popup. If more than one listeners return True a user defined priority list (changeable by settings) is used to decide for which EventListener to call an on_show_popup() method which a package uses to actually prepares and shows the popup.

Means all the stuff in the today's on_hover() method which is used to decide whether to show a popup or not is put into the on_request_popup() method and the rest which actually creates and shows the popup is put into the on_show_popup().

By splitting on_hover() this way, each package has the chance to provide the requested handshake to avoid popup fighting on the fly. No need for the dependency to implement all the required checks of regions or scopes - just managing.

[r-stein]

That sounds nice and could not only work for gutter popups, but for all potential popup conflicts.

I think for the priorities a dict may be better than a list:

"hover_popup_priorities": {
    "Anaconda": 9,
    "GitGutter": 5,
    "SublimeLinter": 8,
    //...
}

[deathaxe]

Sounds good.

What if a package implements several event listeners which are meant to show popups? Call all on_show_popup() methods of that plugin and let it decide?

How about the on_request_popup() to return a priority?

If "hover_popup_priorities" does not contain a package's value the default one returned by on_request_popup() could be used to add an entry.

Alternatively the "hover_popup_priorities" could be omitted and the priority returned by on_request_popup() be used only. A package would need to add a package specific setting to let the user change its priority.

[r-stein]

If a package has several popup I would just add several settings, e.g.

"hover_popup_priorities": {
    "GitGutter.diff_popup": 4,
    "GitGutter.revert_popup": 5,
    // ...
}

Not sure how to set the default priority, but we could maybe add a extra method popup_priority, which is called and initializes the value if it is not present (we also need to get the name from somewhere). My motivation for such a setting is that the user can decide which popup should be shown, e.g. someone may prefer a Linter of an other one or even GitGutter.