Merge lp://qastaging/~mhr3/unity-lens-files/folder-fixes into lp://qastaging/unity-lens-files

Proposed by Michal Hruby
Status: Merged
Merged at revision: 190
Proposed branch: lp://qastaging/~mhr3/unity-lens-files/folder-fixes
Merge into: lp://qastaging/unity-lens-files
Diff against target: 245 lines (+103/-45)
2 files modified
src/daemon.vala (+50/-40)
src/folder.vala (+53/-5)
To merge this branch: bzr merge lp://qastaging/~mhr3/unity-lens-files/folder-fixes
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Review via email: mp+76525@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2011-09-21.

Description of the change

Fixes Folder filter and bookmarks displaying deleted directories.

To post a comment you must log in.
Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal

I can confirm that it works :-) However, I'd like to refactor this a bit. The idea so far has been to keep all bookmarks logic contained in the Bookmarks class in folders.vala.

I suggest that the filtering be moved into that class an automatically taken care of when you call bookmarks.prefix_search() or bookmarks.list().

Also - there's no reason to do async IO here since we have no UI to block :-). In fact async IO is to be taken cautiously inside the lens daemons because it warrants the need for reentrancy safety for fast changing queries (the daemons protect somewhat against this at a higher level, but still).

Otherwise good!

review: Needs Fixing
Revision history for this message
Michal Hruby (mhr3) wrote : Posted in a previous version of this proposal

> I can confirm that it works :-) However, I'd like to refactor this a bit. The
> idea so far has been to keep all bookmarks logic contained in the Bookmarks
> class in folders.vala.
>
> I suggest that the filtering be moved into that class an automatically taken
> care of when you call bookmarks.prefix_search() or bookmarks.list().
>

Fair enough, I'll fix that.

> Also - there's no reason to do async IO here since we have no UI to block :-).
> In fact async IO is to be taken cautiously inside the lens daemons because it
> warrants the need for reentrancy safety for fast changing queries (the daemons
> protect somewhat against this at a higher level, but still).
>
> Otherwise good!

TBH, I'm not sure doing everything synchronously is such a good idea, true we do not have a UI to block, but we make the subsequent queries execute slower, imo the correct solution would be to add a Cancellable instance to each query and call cancel on it if the lens gets a new query. But perhaps this would be something for P.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

ok, I like :-)

There's a small gotcha - that we can address in a later patch: When showing all folder I can see my "ratings-query" directory. But when searching for "rat" it doesn't show in the result set. I think we may be matching against the wrong field...

review: Approve

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