Get rid of objreg? #640

Open
The-Compiler opened this Issue Apr 17, 2015 · 5 comments

Comments

3 participants
@The-Compiler
Collaborator

The-Compiler commented Apr 17, 2015

qutebrowser currently has a global object registry, where objects are registered and retrieved using a string as name - see the contributing documentation for some details about it. This is a stringly typed API and got us into various problems (like #1638, #1652, and bad testability).

This should be phased out and replaced by global objects where appropriate (like modeman.instance, config.instance) and objects passed as arguments elsewhere.

There seem to be various uses of the object registry right now:

Commands

We somehow need to get the proper instance to registered commands. Currently this is done via instance='some-string'[, scope={'tab','window'}] passed to @cmdutils.register, and Command then uses that and gets the instance via the object registry.

I'm still not 100% sure if this will work or not, but we probably could change that to something like this:

  • There are three different "instance registries" - a global one, one for each mainwindow, and one for each tab. Those are only accessible from commands.
  • The application/window/tab is responsible to register its children there.
  • When a command arrives, some central got_command signal is emitted.
  • There's a global/mainwindow/tab CommandDispatcher (?) which gets the got_command signal and calls all command handlers with the correct instances.

Global objects

These are the current global objects:

app, args, cache, command-history, config, cookie-jar, debug-console, host-blocker, ipc-server, js-bridge, key-config, quickmark-manager, save-manager, session-manager, state-config, web-history

Alternatives:

  • Pass stuff as arguments instead
  • Use global python objects with convenience methods (like config.get, etc.).
  • Attach them to the Application (e.g. args) and use QApplication.instance() (or a convenience function for that).

Getting other objects

Many of those probably could be replaced by instance variables or signals/slots.

rcorre added a commit to rcorre/qutebrowser that referenced this issue Jul 28, 2016

Don't use objreg to get CompletionView.
The CompletionView is the parent of the Completer, so there's no need
to use objreg to get it.
See #640.

rcorre added a commit to rcorre/qutebrowser that referenced this issue Jul 28, 2016

Don't use objreg to get CompletionView.
The CompletionView is the parent of the Completer, so there's no need
to use objreg to get it.
See #640.
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Sep 9, 2016

Collaborator

I had some ideas on how to handle this for commands inside classes, which will
probably also solve #1620 (cc @rcorre).

  • When @cmdutils.register gets called on a class (maybe just look for an
    argument named self? Or have a second decorator?), it just stores something
    in a _qutebrowser_commands attribute on the class, without actually
    registering the command yet, as we don't know the instance.
  • objreg is no more at that point - instead there's only some kind of
    instance-registry for commands - a global one, one for each window, and one
    for each tab.
  • When using @cmdutils.register on methods, one needs to do
    cmdutils.register_instance(object[, window[, tab]]) or so after
    creating the object (like in an init() function). We hopefully don't need a
    name anymore here, and just can use the class object as key. This will
    enforce there's only one registered instance (or one per window/tab) too.
  • cmdutils.register_instance checks the _qutebrowser_commands attribute and
    then actually registers the commands, also knowing the right instance
  • We could have a cmdutils.check_instances() or so called at the end of
    initialization, which verifies that all registered commands actually have a
    registered instance.

This also means it's much easier to have some commands in a class when we need
to store some state, like I just imagined for #725:

class RepeatCommand:

    def __init__(self):
        self._timers = []

    @cmdutils.register()
    def repeat(self, times=None, command=None, stop=False):
        if stop:
            for timer in self._timers:
                timer.stop()
        else:
            # ...


def init():
    cmdutils.register_instance(RepeatCommand())

This could also be a quite nice way to register commands inside classes for
plugins in the future.

Compared to now:

class RepeatCommand:

    # ...

    @cmdutils.register(instance='repeat-command')
    def repeat(self, times=None, command=None, stop=False):
        # ...


def init():
    objreg.register('repeat-command', RepeatCommand())

This is slightly more verbose, but most importantly, it leaves an object in the
objreg we want to get rid of, and also one we don't need anywhere else anyways.
For plugins, this also needs an unique name (on the other hand, the command
name needs to be unique anyways...).

Collaborator

The-Compiler commented Sep 9, 2016

I had some ideas on how to handle this for commands inside classes, which will
probably also solve #1620 (cc @rcorre).

  • When @cmdutils.register gets called on a class (maybe just look for an
    argument named self? Or have a second decorator?), it just stores something
    in a _qutebrowser_commands attribute on the class, without actually
    registering the command yet, as we don't know the instance.
  • objreg is no more at that point - instead there's only some kind of
    instance-registry for commands - a global one, one for each window, and one
    for each tab.
  • When using @cmdutils.register on methods, one needs to do
    cmdutils.register_instance(object[, window[, tab]]) or so after
    creating the object (like in an init() function). We hopefully don't need a
    name anymore here, and just can use the class object as key. This will
    enforce there's only one registered instance (or one per window/tab) too.
  • cmdutils.register_instance checks the _qutebrowser_commands attribute and
    then actually registers the commands, also knowing the right instance
  • We could have a cmdutils.check_instances() or so called at the end of
    initialization, which verifies that all registered commands actually have a
    registered instance.

This also means it's much easier to have some commands in a class when we need
to store some state, like I just imagined for #725:

class RepeatCommand:

    def __init__(self):
        self._timers = []

    @cmdutils.register()
    def repeat(self, times=None, command=None, stop=False):
        if stop:
            for timer in self._timers:
                timer.stop()
        else:
            # ...


def init():
    cmdutils.register_instance(RepeatCommand())

This could also be a quite nice way to register commands inside classes for
plugins in the future.

Compared to now:

class RepeatCommand:

    # ...

    @cmdutils.register(instance='repeat-command')
    def repeat(self, times=None, command=None, stop=False):
        # ...


def init():
    objreg.register('repeat-command', RepeatCommand())

This is slightly more verbose, but most importantly, it leaves an object in the
objreg we want to get rid of, and also one we don't need anywhere else anyways.
For plugins, this also needs an unique name (on the other hand, the command
name needs to be unique anyways...).

@Kingdread

This comment has been minimized.

Show comment
Hide comment
@Kingdread

Kingdread Sep 9, 2016

Collaborator

When @cmdutils.register gets called on a class (maybe just look for an argument named self? Or have a second decorator?), it just stores something in a _qutebrowser_commands attribute on the class, without actually registering the command yet, as we don't know the instance.

That won't work in this way, as the class is not yet created when the class methods are defined, and the decorator won't have access to the class. We can only add some attribute to the function itself, and later iterate over all class members, taking the ones we want.

Collaborator

Kingdread commented Sep 9, 2016

When @cmdutils.register gets called on a class (maybe just look for an argument named self? Or have a second decorator?), it just stores something in a _qutebrowser_commands attribute on the class, without actually registering the command yet, as we don't know the instance.

That won't work in this way, as the class is not yet created when the class methods are defined, and the decorator won't have access to the class. We can only add some attribute to the function itself, and later iterate over all class members, taking the ones we want.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Sep 9, 2016

Collaborator

Oh, indeed - that still sounds like a good plan though.

Collaborator

The-Compiler commented Sep 9, 2016

Oh, indeed - that still sounds like a good plan though.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Feb 14, 2018

Collaborator

@jberglinds asked for something refactoring-related to work on as a group in the KTH Sweden, I'm currently writing a mail proposing this issue. @rcorre, I noticed you started some work back in December in the byeobjreg branch but that seems abandoned - is this okay to take over, or do you plan to continue?

Collaborator

The-Compiler commented Feb 14, 2018

@jberglinds asked for something refactoring-related to work on as a group in the KTH Sweden, I'm currently writing a mail proposing this issue. @rcorre, I noticed you started some work back in December in the byeobjreg branch but that seems abandoned - is this okay to take over, or do you plan to continue?

@rcorre

This comment has been minimized.

Show comment
Hide comment
@rcorre

rcorre Feb 14, 2018

Collaborator
Collaborator

rcorre commented Feb 14, 2018

@The-Compiler The-Compiler added this to Refactoring in HSR SA Mar 21, 2018

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