sublime_lib icon indicating copy to clipboard operation
sublime_lib copied to clipboard

Add SettingsListener.

Open Thom1729 opened this issue 4 years ago • 4 comments

For #164.

Notes:

  • Needs documentation, many more tests, more error handling.
  • A couple of the types could be tightened up.
  • OnChangedOptions is a named tuple so that we can easily support more arguments to on_setting_changed.
  • One obvious idea is an async argument to on_setting_changed. Alternatively/additionally, we could have a separate on_setting_changed_async decorator.
  • Instead of SETTINGS_NAME = 'Foo', we could just have settings = sublime.load_settings('Foo.sublime-settings'). I think the former might be slightly less prone to user error.
  • As you mentioned in the issue, it would be nice to have error handling if someone uses the on_setting_changed decorator outside a BaseSettingsListener. Haven't looked into implementation yet.
  • If someone unintentionally exports ViewSettingsListener or GlobalSettingsListener from their plugin, Sublime will dutifully use them. This probably doesn't really matter, but we could special-case this if we want.
  • Is there any reason to let users specify a custom key, rather than str(id(self))? I suspect not.
  • Do we want a WindowSettingsListener? I also suspect not.
  • Currently, selector only supports strings and functions, but we could just use _util.collections.get_selector.

Thom1729 avatar Apr 12 '21 17:04 Thom1729

I've run into a bit of trouble with GlobalSettingsListener. It turns out that EventListeners are never garbage-collected. (I'm pretty sure this is a bug.)

I have solved the problem with a moderately ugly hack. GlobalSettingsListener implements an on_new handler that does nothing. Then, in _on_settings_change, it checks whether that callback is registered. If not, it calls its own __del__.

After spending several hours debugging this, I'm confident that this was the problem and that the solution works — though I wouldn't call it pretty.

(There's no such problem with ViewEventListeners.)

Thom1729 avatar Apr 13 '21 21:04 Thom1729

@FichteFoll Any thoughts on the above hack?

Thom1729 avatar Apr 17 '21 20:04 Thom1729

Regarding the on_new hack: I think it's reasonable, although I surely wish it wasn't necessary. Yet, I cannot think of a better idea, so LGTM in general.

FichteFoll avatar Apr 28 '21 19:04 FichteFoll

I wrote up some docs for the projection function. I'm not completely satisfied with it, but it might be good enough.

Thom1729 avatar May 21 '21 18:05 Thom1729