Merge lp://staging/~submarine/ubuntu-scopes/sshsearch-new-api into lp://staging/~submarine/ubuntu-scopes/sshsearch

Proposed by Mark Tully
Status: Needs review
Proposed branch: lp://staging/~submarine/ubuntu-scopes/sshsearch-new-api
Merge into: lp://staging/~submarine/ubuntu-scopes/sshsearch
Diff against target: 508 lines (+287/-189)
2 files modified
src/unity_sshsearch_daemon.py (+230/-152)
tests/test_sshsearch.py (+57/-37)
To merge this branch: bzr merge lp://staging/~submarine/ubuntu-scopes/sshsearch-new-api
Reviewer Review Type Date Requested Status
James Henstridge Needs Fixing
Review via email: mp+155078@code.staging.launchpad.net

Description of the change

Updating to Unity 7 api

To post a comment you must log in.
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

Unmerged revisions

5. By Mark Tully

Ported to Unity 7 API
Updated tests

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: