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

Multiple event hooks don't work with clones #289

Closed
schlamar opened this issue May 22, 2014 · 17 comments
Closed

Multiple event hooks don't work with clones #289

schlamar opened this issue May 22, 2014 · 17 comments

Comments

@schlamar
Copy link

ST 3062, Win 7

Steps to reproduce:

  1. create plugin which logs view ids in on_post_save and on_activated (see below)
  2. create a file and save
  3. create a clone ("New view into file") and save the clone

Now you can see in the console that on_post_save is called twice with the parent view but not with the clone. As comparison, on_activated is called correctly.

on_selection_modified will print the same id for all clones. (from #349)

Test plugin:

import sublime_plugin

class Listener(sublime_plugin.EventListener):

    def on_post_save(self, view):
        print (view.id())

    def on_activated(self, view):
        print (view.id())

    def on_selection_modified(self, view):
        print (view.id())
@titoBouzout
Copy link
Collaborator

Workaround: view = sublime.active_window().active_view()
you may need to iterate every window, and find for these. .. meanwhile..

@schlamar
Copy link
Author

Workaround

Ah nice, this seems to work (even the simple version).

@titoBouzout
Copy link
Collaborator

on_activated will work always, but on on_post_save will work only if the view has focus. (imagine a Save all action will not get the expected result...)

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jul 4, 2017

Still persists in 3140 (via @deathaxe in duplicate issue #1253).

There is a workaround, but imo it is sub-par as it will fail for modifications that happen to inactive views and is not proper usage of the API.

Note that this also applies to ViewEventListeners.

neilalex added a commit to neilalex/BracketHighlighter that referenced this issue Oct 17, 2017
evandrocoan added a commit to evandrocoan/HighlightWordsOnSelection that referenced this issue Jun 20, 2018
unhighlight when unselecting with arrow keys sometimes,

with workaround from sublimehq/sublime_text#289
Multiple event hooks don't work with clones
@evandrocoan
Copy link

evandrocoan commented Jun 20, 2018

Extending titoBouzout's fix also for output panels and widgets:

New hack into the panel visibility:

class HackListener(sublime_plugin.EventListener):

    def on_selection_modified(self, view):
        active_window = sublime.active_window()
        panel_has_focus = not view.file_name()

        is_widget = view.settings().get('is_widget')
        active_panel = active_window.active_panel()

        print( "is_widget:", is_widget )
        print( "panel_has_focus:", panel_has_focus )
        if active_panel and panel_has_focus or is_widget:
            correct_view = view
        
        else:
            correct_view = active_window.active_view()

This has a flaw, it will not work, if the current view is a new file not saved on the file system, as they will also not report a file name when calling view.file_name(). So, this will also report the panel has focus when you have focus on this new file view.

  1. Forum$8453 How to track if an output panel is closed/hidden?

The correct_view result will the the correct view, even when there is a panels focused.

@FichteFoll
Copy link
Collaborator

Note that you probably shouldn't do anything in your listener if you encounter widgets or panels when you are filtering them out.

@wbond
Copy link
Member

wbond commented Sep 17, 2019

So currently this affects:

  • on_modified()
  • on_selection_modified()
  • on_load()
  • on_pre_save()
  • on_post_save()

The reason is, these are "text buffer" events, and multiple views into a file doesn't change the fact that they all share a single text buffer.

The question, to me, is if these should ever be called on a clone? The ViewEventListener has the concept of applies_to_primary_view_only(). I guess it sort of depends on what you are doing. If you are using a text buffer event to do something to the buffer, you only need a notification on the main view. If you are using the event to do something to the view, then you'd want it on each.

@wbond wbond added this to the Build 3209 milestone Sep 17, 2019
@deathaxe
Copy link
Collaborator

deathaxe commented Sep 17, 2019

Not sure whether all plugins implement the one buffer multiple view infrastructure under all circumstances. GitGutter for instance is organized to work on each view separately. It relies on on_modified() and on_load() to trigger diffing and update the gutter and statusbar. Maybe SublimeLinter and others do the same? I guess PackageDev makes use of that here and there as well, but I am not sure about that.

A plugin should be able to decide whether to handle the events by separate ViewEventListeners with applies_to_primary_view_only() returning True and False depending on the use case.

_ ... or maybe by a BufferEventListener ?_

Another argument might be the type of those events not to be too obvious as the hooks receive a view argument but no buffer.

@ehuss
Copy link

ehuss commented Sep 17, 2019

For my plugin, I'd like on_selection_modified to fire with the view of the clone if the selection was modified in the clone. I display information in the status bar based on the position of the cursor, and if the primary view is given, it is the wrong view to call sel() on when the selection changes in the clone.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Sep 17, 2019

The selection is the property of a view, not a buffer, so on_selection_modified should not only be called once (or most significantly not twice with the same view object). The others are a bit more complicated and rely on how the plugin handles its logic. Buffer ids are rarely something you care for.

The situation has improved greatly with ViewEventListeners, which allow you to track state for a view, but there are still situations they don't handle. For example, if you only need to do something when on_modified is called (which you only need for teh primary view), but you also keep track of some state in the instance. When the primary view gets closed, there is still the clone with the same buffer around that you would like to reuse your old state with. Does the old VEL instance get its self.view silently changed or is the instance destroyed and a new one created?
Regardless, now you need to keep track of global view=>buffer (n->1) relations and monitor on_load and on_close for all views and not just the primary one, which conflicts with the idea of only using a single listener. Also, is a cloned unsaved view that was announced with on_new announced with on_load or on_new as well?

Furthermore, some additional documentation for applies_to_primary_view_only() would also be useful, such as what the default is and whether this means you don't get on_selection_changed events if they occur on a non-primary view.

@evandrocoan
Copy link

evandrocoan commented Sep 18, 2019

The reason is, these are "text buffer" events, and multiple views into a file doesn't change the fact that they all share a single text buffer.

The question, to me, is if these should ever be called on a clone? The ViewEventListener has the concept of applies_to_primary_view_only().

You are right and the current behavior of working with "text buffer" is correct.

However, as was demonstrated by @FichteFoll on #1253 (comment), the actual problem is that the view passed on on_modified and other related events, should be the active view, not the primary view because the view the user is actually seeing is the active view. And with that in mind, this is how probably 100% of Packages Developers think when they are writhing some plugin. The view they are getting is the active view, and not the primary view into the buffer.

As can be seem the first issue (or at least close to the first issue) I opened on some Sublime Text package 3 years ago: facelessuser/BracketHighlighter#356 Highlight not working on multiples files

The package developer just closed the issue as a bug on Sublime Text:

@facelessuser It's really not worth it to me to do an elaborate work around for cloned views. I'm afraid at this point in time I am passing on this.

At the time, nobody understood what was happening and the issue get closed. Only now, we can understand why the plugin was not working. The event was being called, but not with the active view, but with the primary view.

And this is how I fix all my plugins, instead of using the view passed by the on_modified and other related events, I call view.window().active_view() so I can get the actual seeing/visible view and do the changes I would like as add markers, change the caret/cursor selection, add regions, etc.

It should not be required for the primary view to be passed on the on_modified and other related events because if I would like to get the primary view, I should do it by calling some API command like view.primary().

@deathaxe
Copy link
Collaborator

And this is how I fix all my plugins, instead of using the view passed by the on_modified and other related events, I call view.window().active_view() so I can get the actual seeing/visible view ...

Maybe a simple enough workaround for some certain plugins, but I'd expect to always work on the view passed to the event handler - to be sure to work on the expected view.

What if both views are visible, but none is active? The on_modified() triggered by an external file change would then work on the active but unrelated view?

Both the primary view and its clone(s) may be visible at the same time (e.g.: next to each other in a 2-column layout) showing the same lines of the buffer.

Modifying one of the views, causes the diff gutter (internal diff feature) of both views to be updated simultanously.

A plugin should be able to do the same.

A linter for instance calls view.add_region() to highlight warnings/errors in a text. The regions are added to the view, which add_regions() was called for only. Means, such a linter must either iterate through all cloned views to call add_region() or needs to call its logic for each view separately to keep the highlighting synchronized. The latter one would require the events to be sent to all views. The former one would require a simple solution for iterating through all clones of a given primary view by view.get_clones() or something like that.

Either way. The type of event would need a clear documentation.

@kaste
Copy link

kaste commented Sep 18, 2019

To add 2ct. The concept of an active view is misleading here. These are low-level events and can come from any view. In SublimeLinter we usually only care about events from the active view and we have synthesized events for that.

What I don't like is the double, quadruple calling here. (E.g. iirc you click with a mouse you get two 'selection-changed' events per view into the same buffer but all events refer the primary view.) Implementing a throttler/guard all the time is error-prone and seems unnecessary, and honestly most plugins don't do that.

Only for the 'selection-changed' event, calling it with the primary view is a bug.

@evandrocoan
Copy link

What if both views are visible, but none is active?

So far I remember, always I call sublime.window().active_view() always return some view, even if I am focused on a panel, or have multiple groups open. In case I am focused on the some panel, sublime.window().active_view() will not get panel view. So, I have to check if the panel is focused before replacing the view:

try:
    from FixedToggleFindPanel.fixed_toggle_find_panel import is_panel_focused

except ImportError as error:
    print( 'WordHighlightOnSelection Error: Could not import the FixedToggleFindPanel package!', error )

    def is_panel_focused():
        return False

# ...later on
    def on_selection_modified(self, view):
        if not is_panel_focused():
            window = view.window() or sublime.active_window()
            view = window.active_view()

References:

  1. https://github.com/evandrocoan/HighlightWordsOnSelection/blob/master/word_highlight.py#L26
  2. https://github.com/evandrocoan/FixedToggleFindPanel/blob/master/fixed_toggle_find_panel.py#L27

What I don't like is the double, quadruple calling here. (E.g. iirc you click with a mouse you get two 'selection-changed' events per view into the same buffer but all events refer the primary view.)

Lovely put. If I have 10 clones into a view, I get on_selection_modified called 10 times, and each time it always have the primary view as the view parameter. A fix I put to it the other day was this:

# I am saving the state in this class because it is a royal pain in the ass 
# to keep typing `global` every time/everywhere I would like to use a global!
class State(object):
    lasttime = time.time()

    # ...later on
    def on_selection_modified(self, view):
        if view.settings().get('is_widget'):
            return

        timenow = time.time()
        if timenow - State.lasttime < 0.5:
            return
        State.lasttime = timenow

A linter for instance calls view.add_region() to highlight warnings/errors in a text. The regions are added to the view, which add_regions() was called for only. Means, such a linter must either iterate through all cloned views to call add_region() or needs to call its logic for each view separately to keep the highlighting synchronized. The latter one would require the events to be sent to all views. The former one would require a simple solution for iterating through all clones of a given primary view by view.get_clones() or something like that.

I would prefer the latter. Each view on_selection_modified event should be called with their actual view. If I have 10 clone views, there is no need to call 10 times consecutively on_selection_modified passing the primary view as a parameter. (Opened a new issue exclusively for this: on_selection_modified should be called only once and with the view where the selection was modified #2990)

At least for now, only one thing makes sense. This behavior of on_selection_modified calling 10 cloned views, 10 times passing the primary view every time as the view parameter has to change.

To add 2ct. The concept of an active view is misleading here. These are low-level events and can come from any view. In SublimeLinter we usually only care about events from the active view and we have synthesized events for that.

Indeed. This hack just makes the problem goes "unnoticed" for certain plugins and cases.

@deathaxe
Copy link
Collaborator

Createing a list of clones would be possible with existing APIs already. In order to avoid object creation overhead a method View.clones() could look like that:

# sublime.py

class View:

    def clones(self):
        my_buffer_id = self.buffer_id()
        return [
            View(view_id)
            for window_id in sublime_api.windows()
            for view_id in sublime_api.window_views(window_id)
            if view_id != self.view_id and my_buffer_id == sublime_api.view_buffer_id(view_id)
        ]

So far I remember, always I call sublime.window().active_view() always return some view, even if I am focused on a panel,

It's not a question about whether a view is renturned but whether the event_handler acts on the correct or relevant view then.

Each view on_selection_modified event should be called [once] with their actual view.

Agree.

@deathaxe
Copy link
Collaborator

Here is the related PackageDev issue SublimeText/PackageDev#142 suffering from this core issue.

@deathaxe
Copy link
Collaborator

Each cloned view receives the events in question with the correct view object being passed.

It's up to a plugin to handle or ignore the event for all, the active or the primary view only.

class MyListener(sublime_plugin.EventListener):

    def on_reload(self, view):
        print(f"on_reload View {view.id()} : {view.file_name()}.")

    def on_selection_modified(self, view):
        if not view.window():
            return
        if view == view.window().active_view():
            print(f"on_selection_modified called for active View {view.id()} : {view.file_name()}.")
        else:
            print(f"on_selection_modified called for inactive View {view.id()} : {view.file_name()}.")

    def on_post_save(self, view):
        if not view.window():
            return
        if view == view.window().active_view():
            print(f"on_post_save called for active View {view.id()} : {view.file_name()}.")
        else:
            print(f"on_post_save called for inactive View {view.id()} : {view.file_name()}.")

    def on_activated(self, view):
        if not view.window():
            return
        if view == view.window().active_view():
            print(f"on_activated called for active View {view.id()} : {view.file_name()}.")
        else:
            print(f"on_activated called for inactive View {view.id()} : {view.file_name()}.")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants