maryjane
/
syncevolution
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

PIM: add ReplaceSearch, always allow it 

The new ReplaceSearch is more flexible than RefineSearch. It can
handle both tightening the search and relaxing it. The downside of it
is the more expensive implementation (must check all contacts again,
then find minimal set of change signals to update view).

Previously, a search which had no filter set at all at the begining
could not be refined. This limitation of the implementation gets
removed by always using a FilteredView, even if the initial filter is
empty.

When the parent of a FilteredView emits signals, it is not necessarily
always in a consistent state and the FilteredView must not invoke
methods in the parent. Stressing the FilteredView by using it in tests
originally written for the FullView showed that the filling up a view
violated that rule and led to contacts being present multiple
times. Fixed by delaying the reading from the parent into either an
idle callback or the parent's quiescence signal processing, whichever
comes first.
This commit is contained in:
Patrick Ohly 2013-02-25 21:35:46 +01:00
parent 3c21df32e0
commit fde7bc7974
8 changed files with 277 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

209
src/dbus/server/pim/filtered-view.cpp
View File

@ -18,6 +18,9 @@
*/
#include "filtered-view.h"
#include <syncevo/lcs.h>
#include <iterator>
#include <syncevo/BoostHelper.h>
#include <syncevo/declarations.h>
SE_BEGIN_CXX
@ -33,7 +36,21 @@ FilteredView::FilteredView(const boost::shared_ptr<IndividualView> &parent,
void FilteredView::init(const boost::shared_ptr<FilteredView> &self)
{
m_self = self;
m_parent->m_quiescenceSignal.connect(QuiescenceSignal_t::slot_type(boost::bind(boost::cref(m_quiescenceSignal))).track(m_self));
m_parent->m_quiescenceSignal.connect(boost::bind(&FilteredView::parentQuiescent, m_self));
}
void FilteredView::parentQuiescent()
{
// State of the parent is stable again. Check if we queued a "fill view"
// operation and do it now, before forwarding the quiescent signal.
// This gives us the chance to add a contact before a previous remove
// signal is sent, which then enables the combination of two signals
// into one.
if (m_fillViewOnIdle) {
fillViewCb();
m_fillViewOnIdle.deactivate();
}
m_quiescenceSignal();
}
boost::shared_ptr<FilteredView> FilteredView::create(const boost::shared_ptr<IndividualView> &parent,
@ -61,16 +78,19 @@ void FilteredView::doStart()
m_parent->m_removedSignal.connect(ChangeSignal_t::slot_type(boost::bind(&FilteredView::removeIndividual, this, _1, _2)).track(m_self));
}
bool FilteredView::isFull()
bool FilteredView::isFull(const Entries_t local2parent,
const boost::shared_ptr<IndividualFilter> &filter)
{
size_t newEndIndex = m_local2parent.end() - m_local2parent.begin();
return !m_filter->isIncluded(newEndIndex);
size_t newEndIndex = local2parent.end() - local2parent.begin();
return !filter->isIncluded(newEndIndex);
}
void FilteredView::fillView(int candidate)
void FilteredView::fillViewCb()
{
// Can we add back contacts which were excluded because of the
// maximum number of results?
SE_LOG_DEBUG(NULL, NULL, "filtered view %s: fill view on idle", getName());
int candidate = m_local2parent.empty() ? 0 : m_local2parent.back() + 1;
while (!isFull() &&
candidate < m_parent->size()) {
const IndividualData *data = m_parent->getContact(candidate);
@ -79,7 +99,16 @@ void FilteredView::fillView(int candidate)
}
}
void FilteredView::refineFilter(const boost::shared_ptr<IndividualFilter> &individualFilter)
void FilteredView::fillView()
{
if (!m_fillViewOnIdle) {
m_fillViewOnIdle.runOnce(-1,
boost::bind(&FilteredView::fillViewCb, this));
}
}
void FilteredView::replaceFilter(const boost::shared_ptr<IndividualFilter> &individualFilter,
bool refine)
{
// Keep number of results the same, to avoid additional corner
// cases.
@ -89,31 +118,154 @@ void FilteredView::refineFilter(const boost::shared_ptr<IndividualFilter> &indiv
}
individualFilter->setMaxResults(m_filter->getMaxResults());
int candidate = m_local2parent.empty() ? 0 : m_local2parent.back() + 1;
bool removed = false;
size_t index = 0;
while (index < m_local2parent.size()) {
const IndividualData *data = m_parent->getContact(m_local2parent[index]);
if (individualFilter->matches(*data)) {
// Still matched, just skip it.
++index;
} else {
// No longer matched, remove it.
m_local2parent.erase(m_local2parent.begin() + index);
m_removedSignal(index, *data);
removed = true;
if (refine) {
// Take advantage of the hint that the search is more strict:
// we know that we can limit searching to the contacts which
// already matched the previous search.
bool removed = false;
size_t index = 0;
while (index < m_local2parent.size()) {
const IndividualData *data = m_parent->getContact(m_local2parent[index]);
if (individualFilter->matches(*data)) {
// Still matched, just skip it.
++index;
} else {
// No longer matched, remove it.
m_local2parent.erase(m_local2parent.begin() + index);
m_removedSignal(index, *data);
removed = true;
}
}
}
m_filter = individualFilter;
m_filter = individualFilter;
if (removed) {
fillView(candidate);
if (removed) {
fillView();
}
} else {
// Brute-force approach.
//
// Here is an example of old and new mapping:
// index into local2parent old value new value
// 0 10 10
// 1 20 30
// 2 30 40
// 3 50 50
// 4 70 60
// 5 - 70
// 6 - 80
//
// The LCS (see below) is:
// (0, 0, 10) (2, 1, 30) (3, 3, 50) (4, 5, 70)
//
// The expected change signals for this transition are:
// "removed", 1
// "added", 2
// "added", 4
// "added", 6
//
// Note that this example does not include all corner cases.
// Also relevant is adding or removing multiple entries at the
// same index.
//
// One could also emit a "modified" signal for each index if
// it is different, but then a single insertion or deletion
// would led to invalidating the entire view.
//
// 1. build new result list.
Entries_t local2parent;
int candidate = 0;
while (!isFull(local2parent, individualFilter) &&
candidate < m_parent->size()) {
const IndividualData *data = m_parent->getContact(candidate);
if (individualFilter->matches(*data)) {
local2parent.push_back(candidate);
}
candidate++;
}
// 2. morph existing one into new one.
//
// Uses the SyncEvolution longest-common-subsequence
// algorithm. Because all entries are different, there can be
// only one solution and thus there is no need for a cost
// function to find "better" solutions.
std::vector< LCS::Entry<int> > common;
common.reserve(std::min(m_local2parent.size(), local2parent.size()));
LCS::lcs(m_local2parent, local2parent, std::back_inserter(common), LCS::accessor_sequence<Entries_t>());
// To emit the discovered changes as "added" and "removed"
// signals, we need to look at identical entries.
// The "delta" here always represents the value "new = b" -
// "old = a" which needs to be added to the old index to get
// the new one. It summarizes the change signals emitted so
// far. For each common entry, we need to check if the delta
// is different from what we have told the agent so far.
int delta = 0;
// The "shift" is the "old modified" - "old original" that
// tells us how many entries were added (positive value) or
// removed (negative value) via signals.
int shift = 0;
// Old and new index represent the indices of the previous
// common element plus 1; in other words, the expected next
// common element.
size_t oldIndex = 0,
newIndex = 0;
BOOST_FOREACH (const LCS::Entry<int> &entry, common) {
int new_delta = (ssize_t)entry.index_b - (ssize_t)entry.index_a;
if (delta != new_delta) {
// When emitting "added" or "removed" signals,
// be careful about getting the original index
// in the old set right. It needs to be adjusted
// to include the already sent changes.
if (delta < new_delta) {
size_t change = new_delta - delta;
for (size_t i = 0; i < change; i++) {
const IndividualData *data = m_parent->getContact(local2parent[newIndex + i]);
// Keep adding at new indices, one new element
// after the other.
m_addedSignal(oldIndex - shift + i, *data);
}
shift -= change;
} else if (delta > new_delta) {
size_t change = delta - new_delta;
shift += change;
for (size_t i = 0; i < change; i++) {
size_t index = oldIndex - shift;
const IndividualData *data = m_parent->getContact(m_local2parent[index + i]);
// Keep removing at the same index, because
// that's how it'll look to the recipient.
m_removedSignal(index, *data);
}
}
new_delta = delta;
}
oldIndex = entry.index_a + 1;
newIndex = entry.index_b + 1;
}
// Now deal with entries after the latest common entry, in
// both arrays.
for (size_t index = oldIndex; index < m_local2parent.size(); index++) {
const IndividualData *data = m_parent->getContact(m_local2parent[index]);
m_removedSignal(oldIndex - shift, *data);
}
for (size_t index = newIndex; index < local2parent.size(); index++) {
const IndividualData *data = m_parent->getContact(local2parent[index]);
m_addedSignal(index, *data);
}
// - swap
std::swap(m_local2parent, local2parent);
}
// If the parent is currently busy, then we can delay sending the
// signal until it is no longer busy.
if (isQuiescent()) {
m_quiescenceSignal();
parentQuiescent();
}
}
@ -192,8 +344,10 @@ void FilteredView::removeIndividual(int parentIndex, const IndividualData &data)
SE_LOG_DEBUG(NULL, NULL, "%s: removed at #%ld/%ld", getName(), (long)index, (long)m_local2parent.size());
m_local2parent.erase(it);
m_removedSignal(index, data);
// Try adding more contacts from the parent if there is room now.
fillView(parentIndex);
// Try adding more contacts from the parent once the parent
// is done with sending us changes - in other words, wait until
// the process is idle.
fillView();
}
}
@ -214,10 +368,9 @@ void FilteredView::modifyIndividual(int parentIndex, const IndividualData &data)
} else {
// Removed.
SE_LOG_DEBUG(NULL, NULL, "%s: removed at #%ld/%ld due to modification", getName(), (long)index, (long)m_local2parent.size());
int candidate = m_local2parent.back() + 1;
m_local2parent.erase(it);
m_removedSignal(index, data);
fillView(candidate);
fillView();
}
} else if (matches) {
// Was not matched before and is matched now => add it.

20
src/dbus/server/pim/filtered-view.h
View File

@ -48,8 +48,21 @@ class FilteredView : public IndividualView
const boost::shared_ptr<IndividualFilter> &filter);
void init(const boost::shared_ptr<FilteredView> &self);
bool isFull();
void fillView(int candidate);
bool isFull() const { return isFull(m_local2parent, m_filter); }
static bool isFull(const Entries_t local2parent,
const boost::shared_ptr<IndividualFilter> &filter);
/**
* Request filling up the filtered view once things are stable again.
*/
void fillView();
/**
* internal helper for fillView(), do not call directly
*/
void fillViewCb();
Timeout m_fillViewOnIdle;
void parentQuiescent();
public:
/**
@ -82,7 +95,8 @@ class FilteredView : public IndividualView
// from IndividualView
virtual void doStart();
virtual void refineFilter(const boost::shared_ptr<IndividualFilter> &individualFilter);
virtual void replaceFilter(const boost::shared_ptr<IndividualFilter> &individualFilter,
bool refine);
virtual int size() const { return (int)m_local2parent.size(); }
virtual const IndividualData *getContact(int index) { return (index >= 0 && (unsigned)index < m_local2parent.size()) ? m_parent->getContact(m_local2parent[index]) : NULL; }
};

6
src/dbus/server/pim/full-view.cpp
View File

@ -260,7 +260,7 @@ void FullView::removeIndividual(FolksIndividual *individual)
void FullView::onIdle()
{
SE_LOG_DEBUG(NULL, NULL, "process is idle");
SE_LOG_DEBUG(NULL, NULL, "full view: process is idle");
// Process delayed contact modifications.
BOOST_FOREACH (const FolksIndividualCXX &individual,
@ -279,7 +279,9 @@ void FullView::onIdle()
void FullView::waitForIdle()
{
if (!m_waitForIdle) {
m_waitForIdle.runOnce(-1, boost::bind(&FullView::onIdle, this));
// Run this after all other idle callbacks that we may have added,
// like the "fill view on idle" callback in the filtered view.
m_waitForIdle.runOnce(boost::bind(&FullView::onIdle, this), Timeout::PRIORITY_LOW);
}
}

15
src/dbus/server/pim/locale-factory-boost.cpp
View File

@ -519,15 +519,14 @@ public:
}
}
// May be empty (unfiltered). If a limit was given, then
// we have to create a filter which matches everything, because
// the FullView cannot apply a limit.
if (maxResults != -1) {
if (!res) {
res.reset(new MatchAll());
}
res->setMaxResults(maxResults);
// May be empty (unfiltered). Create a filter which matches
// everything, because otherwise we end up using the FullView,
// which cannot apply a limit or later switch to a different
// search.
if (!res) {
res.reset(new MatchAll());
}
res->setMaxResults(maxResults);
return res;
}

31
src/dbus/server/pim/manager.cpp
View File

@ -654,6 +654,7 @@ class ViewResource : public Resource, public GDBusCXX::DBusObjectHelper
add(this, &ViewResource::readContacts, "ReadContacts");
add(this, &ViewResource::close, "Close");
add(this, &ViewResource::refineSearch, "RefineSearch");
add(this, &ViewResource::replaceSearch, "ReplaceSearch");
activate();
// The view might have been started already, for example when
@ -761,11 +762,13 @@ public:
/** ViewControl.RefineSearch() */
void refineSearch(const LocaleFactory::Filter_t &filter)
{
if (filter.empty()) {
SE_THROW("New filter is empty. It must be more restrictive than the old filter.");
}
replaceSearch(filter, true);
}
void replaceSearch(const LocaleFactory::Filter_t &filter, bool refine)
{
boost::shared_ptr<IndividualFilter> individualFilter = m_locale->createFilter(filter);
m_view->refineFilter(individualFilter);
m_view->replaceFilter(individualFilter, refine);
}
};
unsigned int ViewResource::m_counter;
@ -833,17 +836,17 @@ void Manager::doSearch(const ESourceRegistryCXX &registry,
view = m_folks->getMainView();
bool quiescent = view->isQuiescent();
std::string ebookFilter;
if (!filter.empty()) {
boost::shared_ptr<IndividualFilter> individualFilter = m_locale->createFilter(filter);
ebookFilter = individualFilter->getEBookFilter();
if (quiescent) {
// Don't search via EDS directly because the unified
// address book is ready.
ebookFilter.clear();
}
view = FilteredView::create(view, individualFilter);
view->setName(StringPrintf("filtered view%u", ViewResource::getNextViewNumber()));
// Always use a filtered view. That way we can implement ReplaceView or RefineView
// without having to switch from a FullView to a FilteredView.
boost::shared_ptr<IndividualFilter> individualFilter = m_locale->createFilter(filter);
ebookFilter = individualFilter->getEBookFilter();
if (quiescent) {
// Don't search via EDS directly because the unified
// address book is ready.
ebookFilter.clear();
}
view = FilteredView::create(view, individualFilter);
view->setName(StringPrintf("filtered view%u", ViewResource::getNextViewNumber()));
SE_LOG_DEBUG(NULL, NULL, "preparing %s: EDS search term is '%s', active address books %s",
view->getName(),

7
src/dbus/server/pim/pim-manager-api.txt
View File

@ -354,6 +354,13 @@ Methods:
to the view when setting a less restrictive filter (simplifies
the implementation and improves performance).
void ReplaceSearch(list filter, bool refine)
Same as RefineSearch() if refine is true. If refine is false,
the new filter can be less restrictive and contacts which
did not match the old filter will be added back to the list
of matching contacts.
Service: [user of the PIM Manager]
Interface: org._01.pim.contacts.ViewAgent

57
src/dbus/server/pim/testpim.py
View File

@ -1766,11 +1766,9 @@ END:VCARD''' % {'peer': peer, 'index': index})
'''TestContacts.testFilterExisting - check that filtering works when applied to static contacts'''
self.setUpView()
# Cannot refine full view.
with self.assertRaisesRegexp(dbus.DBusException,
r'.*: refining the search not supported by this view$'):
self.view.view.RefineSearch([['any-contains', 'foo']],
timeout=self.timeout)
# Can refine full view. Doesn't change anything here.
self.view.view.RefineSearch([],
timeout=self.timeout)
# Override default sorting.
self.assertEqual("last/first", self.manager.GetSortOrder(timeout=self.timeout))
@ -1848,11 +1846,19 @@ END:VCARD''']):
until=lambda: view.haveData(0))
self.assertEqual(u'Charly', view.contacts[0]['structured-name']['given'])
# Cannot expand view.
with self.assertRaisesRegexp(dbus.DBusException,
r'.*: New filter is empty. It must be more restrictive than the old filter\.$'):
view.view.RefineSearch([],
timeout=self.timeout)
# We can expand the search with ReplaceSearch().
view.view.ReplaceSearch([], False,
timeout=self.timeout)
self.runUntil('expanded view with three contacts',
check=lambda: self.assertEqual([], self.view.errors),
until=lambda: len(self.view.contacts) == 3)
self.view.read(0, 3)
self.runUntil('expanded contacts',
check=lambda: self.assertEqual([], self.view.errors),
until=lambda: self.view.haveData(0, 3))
self.assertEqual(u'Abraham', self.view.contacts[0]['structured-name']['given'])
self.assertEqual(u'Benjamin', self.view.contacts[1]['structured-name']['given'])
self.assertEqual(u'Charly', self.view.contacts[2]['structured-name']['given'])
# Find Charly by his FN (case insensitive explicitly).
view = ContactsView(self.manager)
@ -1903,14 +1909,15 @@ END:VCARD''']):
self.assertEqual(u'Benjamin', view.contacts[1]['structured-name']['given'])
# Refine search without actually changing the result.
view.quiescentCount = 0
view.view.RefineSearch([['any-contains', 'am']],
timeout=self.timeout)
self.runUntil('end of search refinement',
check=lambda: self.assertEqual([], view.errors),
until=lambda: view.quiescentCount > 0)
self.assertEqual(u'Abraham', view.contacts[0]['structured-name']['given'])
self.assertEqual(u'Benjamin', view.contacts[1]['structured-name']['given'])
for refine in [True, False]:
view.quiescentCount = 0
view.view.ReplaceSearch([['any-contains', 'am']], refine,
timeout=self.timeout)
self.runUntil('end of search refinement',
check=lambda: self.assertEqual([], view.errors),
until=lambda: view.quiescentCount > 0)
self.assertEqual(u'Abraham', view.contacts[0]['structured-name']['given'])
self.assertEqual(u'Benjamin', view.contacts[1]['structured-name']['given'])
# Restrict search to Benjamin. The result is a view
# which has different indices than the full view.
@ -2526,6 +2533,20 @@ END:VCARD''']):
until=lambda: view.quiescentCount > 0)
self.assertEqual(0, len(view.contacts))
# Expand back to view with Benjamin.
view.quiescentCount = 0
view.view.ReplaceSearch([['any-contains', 'Benjamin']], False,
timeout=self.timeout)
self.runUntil('end of search replacement',
check=lambda: self.assertEqual([], view.errors),
until=lambda: view.quiescentCount > 0)
self.assertEqual(1, len(view.contacts))
view.read(0, 1)
self.runUntil('Benjamin',
check=lambda: self.assertEqual([], view.errors),
until=lambda: view.haveData(0))
self.assertEqual(u'Benjamin', view.contacts[0]['structured-name']['given'])
@timeout(60)
@property("ENV", "LC_TYPE=de_DE.UTF-8 LC_ALL=de_DE.UTF-8 LANG=de_DE.UTF-8")
def testFilterLiveLimit(self):

9
src/dbus/server/pim/view.h
View File

@ -123,11 +123,12 @@ class IndividualView : public View
ChangeSignal_t m_modifiedSignal;
/**
* Replace filter with more specific one. Only supported by a view
* which is already filtered.
* Replace filter with more specific one (refine = true) or redo
* search without limitations.
*/
virtual void refineFilter(const boost::shared_ptr<IndividualFilter> &individualFilter) {
SE_THROW("refining the search not supported by this view");
virtual void replaceFilter(const boost::shared_ptr<IndividualFilter> &individualFilter,
bool refine) {
SE_THROW("adding a search not supported by this view");
}
/** current number of entries */