Code review comment for lp://staging/~submarine/ubuntu-scopes/sshsearch-new-api

Revision history for this message
James Henstridge (jamesh) wrote :

This scope is already working with Libunity 7, right? The diff shows it using Unity.DeprecatedScope, which while it isn't the new recommended new API it will work.

From the look of it, the two file monitors set up in the search() routine don't do anything. For example, the new read_config() function appears to have no side effects, so the following code looks like it won't do anything useful:

    + config_file = Gio.file_new_for_path(SSHCONFIG_EXPAND)
    + config_monitor = config_file.monitor_file(flags=Gio.FileMonitorFlags.NONE, cancellable=None)
    + config_monitor.connect('changed', read_config)

The original code doesn't look much better, setting up the file monitors and rereading config files every time update_results_model is called.

If the intent is to only parse the files when they change, I'd expect the scope to set up the file monitors once on start up, and use the cached config data to respond to searches.

Also, constructs like the following are difficult to read:

    + if not 'uri' in i or not i['uri'] or i['uri'] == '':

The following should provide the same result:

    if not i.get('uri'):

With the if statements shortened, you should have space to use a more meaningful identifier than 'i' too.

review: Needs Fixing

« Back to merge proposal