Skip to content

Conversation

@Thom1729
Copy link
Member

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 Thom1729 changed the title Add SettingsListener. [WIP] Add SettingsListener. Apr 12, 2021
@Thom1729
Copy link
Member Author

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
Copy link
Member Author

@FichteFoll Any thoughts on the above hack?

If `selector` is callable, then the derived value is ``selector(self)``.
If `selector` is a :class:`str`,
then the derived value is ``self.get(selector, None)``.
Otherwise, the derived value is ``projection(self, selector)``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the "selector" concept being reused now, it might be sensible to add a section to our docs with more details and examples.

@FichteFoll
Copy link
Member

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 FichteFoll added this to the 1.6.0 milestone Apr 28, 2021
Copy link
Member

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think that was everything.

Edit: Oh, I forgot about the documentation thing. I'd still like to see that.

@Thom1729
Copy link
Member Author

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

@Thom1729 Thom1729 changed the title [WIP] Add SettingsListener. Add SettingsListener. Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants