Completion model/view revamping

[The-Compiler]

The current way the completion model and view is handled might be improved. Some ideas:

• List models for completion lists, and a QAbstractProxyModel which converts it into a tree model
• List models with a QHBoxLayout of list views

[ff2000]

AFAIK joining several models in one meta model is extremely hard, possibly impossible...
I know that this was the approach Trojita devs wanted to take to implement multiple Mail Accounts: They have one "Imap Model" and thought of "simply" joining them in one Meta Model. That was well over two years ago - and Trojita still has no multiple accounts support ;)

Using several Views and layout them sounds ugly to me - especially filtering could get a performance/maintainance issue.

What about this approach:

• Base class for a model source 'SourceBase' - History, Buffers, Commands, ...
• Base class for an item 'ItemBase' - instances will end up as internalPointer in the model
• CompletionModel will store instances to SourceBase-derived classes (HistorySource etc.)
• A light Filter Model could create a "view" only on needed Sources - :buffer should only complete buffers.

Instead of the last point we could create separate models for each task - as the logic is factored out into sources this could be a single class that e.g. accepts a list of source names it should complete.

Don't know how sane this is... At least it would seperate tasks but still create one single model+view (in contrast to multiple models merged into one model or show multiple views).

[The-Compiler]

Some links I found about joining models:

• Jongling Qt models 2, Composition Gem - A proxymodel which joins all rows of its source models, something similiar to what we want.
• Qt Centre - View using multiple models
• Qt Forum - Display several separate table views as a tree - I mirrored the example at food.zip in case it goes missing.

So it certainly seems possible. I don't really understand your proposotion - is it to have all completions in a single model and then filtering the view accordingly?

After all, the point should be to be able to write new completion models more easily, ideally as a table model. Why do you think it'd be a performance/maintainance issue to filter the sub models? You just apply the CompletionFilterModel for each source model individually, and set the filter pattern for each one. This also has the benefit of being able to choose filter strategies per source model.

[The-Compiler]

I've now opened a question on StackOverflow to see if there's some easier way I haven't found yet.

[rcorre]

Here's the battle plan:

1. Implement the module completion.category. This will contain a function for
   each 'category' of completions. @cmdutils.argument.completion will be a
   list of functions. For example:

   @cmdutils.argument('topic', completion=[completion.category.command,
                                          completion.category.helptopic,
                                          completion.category.setting])
   def show_help(self, tab=False, bg=False, window=False, topic=None):
      ...
   
   
   @cmdutils.argument('command', completion=usertypes.Completion.command)
   def bind(self, key, win_id, command=None, *, mode='normal', force=False):
      ...
   

   Each of these functions will return a CompletionCategory populated with
   items. Ideally the return value would just be a list of items, but we need a
   way to provide an implementation of delete_cur_item for certain categories.
   This is why the CompletionCategory class will exist*.

2. Move completion.model.sortfilter to completion.sortfilter.

3. Replace the completion.model.* package with a single completion.model
   module. This will provide a single CompletionModel class. The Completer
   constructs a CompletionModel by handing it a list of categories generated
   by invoking the functions from the current argument's completion list.
   This also implies eliminating completion.model.instances.

Here are the advantages I anticipate:

1. Less code involved with completion.

2. No mandatory caching.

   Currently, every kind of completion has to go through this registration
   process in completion.model.instances. Not only does this make it harder to
   understand and add new completions, but it is easy to miss hooking up some
   signal needed to keep the completion up to date (see Fix two alias-related bugs #1816). Most kinds of
   completion should be cheap enough to generate on the fly.

   Caching may be necessary for URL completion, but this can be handled within
   the function for completion.category.url_history (and maybe bookmarks and
   quickmarks as well) and need not influence the API.

3. No more usertypes.Completion. This isn't a huge deal, but it was just one
   more module involved with completion. Referencing functions directly
   eliminates this extra indirection.

4. Easier to add new completions. See 2 and 3; you now need to modify only 1
   module (completion.categories) instead of 3 (completion.model.*,
   completion.model.instances, utils.usertypes).

5. Less modules, flatter module structure. completion will now have only 1
   level.

6. Easier to test, mostly due to the elimination of instances.

7. Less redundancy of completion code. Now commands that share categories (e.g.
   command completion) can just reference the same function. Some have slight
   differences in the way they display those categories (e.g. help prefixes
   commands with : while bind doesn't); we'll have to decide whether to
   eliminate those differences or have two slightly different functions (but not
   the same function with different arguments. I don't want to see
   funtools.partial appear in @cmdutils.argument >.<)

* I guess instead of functions they could be subclasses of
CompletionCategory. delete_cur_item would be an overridden function (as it
is now) as opposed to a lambda that gets set by completion functions that want
to implement deletion. I dunno, the lambda seemed cleaner to me but it just
occurred to me that inheritance might be more traditional.

[rcorre]

Well, I can say that looked a lot shorter in open-editor mode 😁.

Here's an abridged version:

1. @cmdutils.argument.completion lists categories, not a single completion
   model. These categories are referenced directly, there's no enum.
2. Burn completion.model.instances to the ground. Only cache when performance
   demands.

[rcorre]

Just realized that in order to handle setting option/value completion, we may need to pass along the current positional args to construct the completer. This could later be extended to pass along kwargs as well, which would allow bind to scope its command completion if given --mode

[The-Compiler]

That sounds good! Some comments:

• I don't understand what you mean with "as opposed to a lambda that gets set by completion functions that want to implement deletion". Where would they set it on? From what you said, it sounded to me like every completion function would be a CompletionCategory class with some kind of items attribute and a delete_cur_item method, both set by the completion function. Is that what you were saying?
• As you mentioned, completion functions need some information about the commandline entered. The nicest way this could be handled is by using parse_known_args with argparse, which would return a nice argparse.Namespace so the completion function could simply look at args.mode or whatever.
• I'm not 100% sure about the caching. Maybe you're right, but maybe it'd also make more sense to have some generic caching in the completion code so we can just turn it on for models where it makes sense, and provide a (list of) signals when it should be invalidated and the completion function called again?

[lahwaacz]

As for the last point, we'd like to cache content and e.g. hints for column widths (see #1584), which will probably be hard to combine in a generic code.

As for ditching completion.model.instances, they are needed whenever you need caching. Since we might want to cache even instances other than models (see e.g. #1654 (comment))), it would be good to have some generic code for this based on e.g. lru_cache. This way you can easily turn it on and off and observe impact on performance.

[rcorre]

For the lambda, I meant something like

# module qutebrowser.completion.category
quickmarks(args*):
    cat = CompletionCategory()
    # ... populate cat with items
    cat.delete_cur_item = delete_quickmark
    return cat

as opposed to:

# module qutebrowser.completion.category
class QuickmarkCompletion(CompletionCategory):
    def __init__(args*):
        # ... populate with items

    def delete_cur_item(index_or_data_or_something):
        # ...

The nicest way this could be handled is by using parse_known_args

Unfortunately, I think parse_known_args will fail if it is missing required
args, so it won't work with a partially typed command.

I'm not 100% sure about the caching

You mean performance-wise?
I suppose it could be made more generic with something like:

class SomeCompletionCategory(CompletionCategory):
    def regenerate_on():
        return [objreg.get("someobj").somesignal,
                objreg.get("otherobj").othersignal]

[rcorre]

Aw crap, I forgot about column widths. lru_cache sounds nice though.

[The-Compiler]

Thanks for the clarification! A lambda is simply a way to create an inline function, but apart from not giving the function a name, it's equivalent to def - which is what confused me. Functions are first class objects in Python, so no matter how you define them, you'll be able to do what you want.

Using functions seems more KISS to me, you could even simply pass a function object to CompletionCategory.__init__, i.e.:

def quickmarks(...):
    cat = CompletionCategory(deletion_func=delete_quickmark)
    # ...
    return cat

Unfortunately, I think parse_known_args will fail if it is missing required args, so it won't work with a partially typed command.

Aw, damn. That'd have been quite handy to have though... I wonder if we're reaching the point where it'd be easier to do our own argument parsing compared to using argparse 😆

You mean performance-wise?

I mainly meant style-wise - I agree caching isn't needed for most completions, but where it is, it might still make more sense to have it generalized and implemented once.

[rcorre]

Using functions seems more KISS to me

Great, I liked that one better too, and its even simpler passing delete_item to the constructor.

I wonder if we're reaching the point where it'd be easier to do our own argument parsing

Yeah, I started to wonder that while trying to implement --mode-specific completions for bind.

I agree caching isn't needed for most completions, but where it is, it might still make more sense to have it generalized and implemented once

Hmm ... maybe. I can play around with both and see which feels better.

I also realized that until we have dynamically-determined column widths from #1584, I probably can't do completions by category, as I need to manually specify COLUMN_WIDTHS for each overall model. That's not a big deal, as none of the other refactoring is dependent on that.

[The-Compiler]

Some more quick thoughts:

• Could you please at least keep Use sqlite internally for completion #1765 in the back of your mind? I still think it'd be a great solution for a performant :open completion, and perhaps even for the other completions where we discussed about caching stuff instead.
• Initially with this issue I imagined having multiple composable completion functions, directly in the @cmdutils.register decorator. On a second thought, I guess if you have completion functions, you could always do:

def setting_value(args):
    return ('Current/Default', _setting_values_special(args),
            'Completions', _setting_values_available(args))

which is nice as well.

[rcorre]

I imagined having multiple composable completion functions

Me as well. That's what I meant by organizing it by category rather than by model. However, I don't think I can do this as long as COLUMN_WIDTHS is manually specified.

[The-Compiler]

Done with #2295.