Gwibber

Merge lp:~robru/gwibber/dee into lp:~barry/gwibber/py3

dee
Merge into py3

Proposed by Robert Bruce Park on 2012-09-18

Status:	Merged
Merged at revision:	1433
Proposed branch:	lp:~robru/gwibber/dee
Merge into:	lp:~barry/gwibber/py3
Diff against target:	2162 lines (+327/-1550) 21 files modified gwibber/gwibber/main.py (+3/-13) gwibber/gwibber/service/dispatcher.py (+2/-2) gwibber/gwibber/service/messages.py (+0/-47) gwibber/gwibber/service/searches.py (+0/-87) gwibber/gwibber/service/streams.py (+0/-93) gwibber/gwibber/storage/messages.py (+0/-172) gwibber/gwibber/storage/searches.py (+0/-63) gwibber/gwibber/storage/streams.py (+0/-68) gwibber/gwibber/testing/com.Gwibber.Messages.service.in (+0/-3) gwibber/gwibber/testing/com.Gwibber.Searches.service.in (+0/-3) gwibber/gwibber/testing/com.Gwibber.Streams.service.in (+0/-3) gwibber/gwibber/testing/dbus.py (+0/-3) gwibber/gwibber/testing/service.py (+1/-48) gwibber/gwibber/tests/test_account.py (+5/-12) gwibber/gwibber/tests/test_dbus.py (+0/-237) gwibber/gwibber/tests/test_model.py (+39/-0) gwibber/gwibber/tests/test_protocols.py (+95/-0) gwibber/gwibber/tests/test_storage.py (+0/-688) gwibber/gwibber/utils/account.py (+7/-6) gwibber/gwibber/utils/base.py (+66/-2) gwibber/gwibber/utils/model.py (+109/-0)
To merge this branch:	bzr merge lp:~robru/gwibber/dee
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Ken VanDine	2012-09-18	Pending
Barry Warsaw	2012-09-18	Pending
Review via email: mp+124827@code.launchpad.net

Description of the change

Implement a Dee.SharedModel with a libgwibber-compatible schema that we can use for passing messages to libgwibber for display, with tests.

Note: Before I wrote this code, I had two terminals open running two interactive python sessions, and I was toying around with Dee.SharedModel. I was not actually able to succeed in getting the two different models to share their data... they never synced up and adding a row from one terminal never resulted in the 'row-added' signal being emitted on the other terminal. None of the documentaton I was able to find offered any help. I was thinking there might need to be some kind of 'flush' method to indicate that the data was ready to be sent over dbus, but I couldn't find any such thing.

So, with that utter failure in mind, I set out to write the simplest possible API that would allow our plugins to "publish" the sanitized json into the model, and this is what I came up with. It works internally, but there is probably something missing before libgwibber is actually able to access this over dbus. Gotta check with kenvandine about this before you approve the merge.

The idea behind my API choice is that all protocol actions will call something like "self._publish(key='word', args=True)" at the end, instead of returning any values. When converting the protocols to use this API, it should be straightforward to mock out _publish with something like "lambda **kwargs: kwargs" just to be able to check the results easily.

lp:~robru/gwibber/dee updated on 2012-09-18

1434. By Robert Bruce Park on 2012-09-18: Fix up a copy&paste error. Oops ;-)
1435. By Robert Bruce Park on 2012-09-18: Stop hard-coding the number of columns in case the schema changes later.

Revision history for this message

Robert Bruce Park (robru) wrote on 2012-09-18:

Also, is Dee threadsafe? Because if it isn't, then we're going to have to give it a threading.Lock instance so that no two threads try to publish at the same time.

lp:~robru/gwibber/dee updated on 2012-09-18

1436. By Robert Bruce Park on 2012-09-18

Rename our SharedModel bus name from com.Gwibber.SharedModel to com.Gwibber.Streams.

This name is more semantically meaningful, rather than being named
after an implementation detail.

1437. By Robert Bruce Park on 2012-09-18

By my estimation, this is all the code that is obsoleted by Dee.SharedModel.

1438. By Robert Bruce Park on 2012-09-18

Test that we can get the message back from the SharedModel, too.

1439. By Robert Bruce Park on 2012-09-18

Schema fixup, plus working de-duplication logic.

The de-dup logic is modelled very closely on the way libgwibber does
it in Vala, but now we're doing it right at the source before
libgwibber even gets there. Three cheers for efficiency!

1440. By Robert Bruce Park on 2012-09-18

Tweak the return value logic in Base._publish.

Also stop hard-coding column index numbers.

1441. By Robert Bruce Park on 2012-09-18

Add a publish_lock to avoid race conditions.

We are checking for duplicates first, and then appending new rows later,
which creates a race condition in between (eg, some other thread can easily
insert a duplicate row after we find that there are no duplicates present, but
before we get around to appending our own row).

This makes duplicate checking / row appends atomic.

1442. By Robert Bruce Park on 2012-09-18

Looks like I didn't rip out enough old crufty stuff last time.

Revision history for this message

Barry Warsaw (barry) wrote on 2012-09-18:

Download full text (18.0 KiB)

I'm going to review the latest and greatest version of the branch.

First of all, this is fantastic! You've done a great job at removing tons of
crufty, legacy, mysterious, horrible old code and replaced it with just a
little bit of good, new, readable code. This is a huge win. Just to show you
how big, here's the diffstat:

You gotta love it when you remove 5x more code than you add, *and* make it so
much more understandable.

One thing I noticed was lots of skipped tests when running under `make
check_all` or a virtualenv. As we discussed on IRC, you're going to fix
those, and I'll review what you've got so far.

I'll skip the deletions and any uncontroversial new code.

=== modified file 'gwibber/gwibber/tests/test_protocols.py'
--- gwibber/gwibber/tests/test_protocols.py 2012-09-13 17:50:35 +0000
+++ gwibber/gwibber/tests/test_protocols.py 2012-09-18 20:37:18 +0000
> @@ -27,6 +27,7 @@
> from gwibber.protocols.twitter import Twitter
> from gwibber.utils.base import Base
> from gwibber.utils.manager import ProtocolManager
> +from gwibber.utils.model import SharedModel
>
>
> class TestProtocolManager(unittest.TestCase):
> @@ -126,6 +127,42 @@
> thread.join()
> self.assertEqual(my_protocol.result, 'one:two')
>
> + def test_publish(self):
> + base = Base(None)
> + with self.assertRaises(TypeError):
> + base._publish(invalid_argument='not good')

I don't think you need the context manager version of this, since you're not
checking anything about the raised exception. You could possibly check
details about the exception, e.g. the bad column name (more on this later),
but if not, then I think this would be fine:

self.assertRaises(TypeError, base._publish, invalid_argument='not good')

One other thing we discussed on IRC is that these tests are not well isolated
from the environment. Because these are not running inside TestDBus, they
actually pollute the user's session. Other than the problem that running the
test would show bogus data in the Gwibber ui, the other problem is that the
model doesn't get reset after every test, so I bet it persists between test
runs. Which means it's not clear this is actually testing what you think it's
testing.

The solution is to move all the _publish() tests to TestDBus, since that
ensures it's ru...

I'm going to review the latest and greatest version of the branch.

First of all, this is fantastic!  You've done a great job at removing tons of
crufty, legacy, mysterious, horrible old code and replaced it with just a
little bit of good, new, readable code.  This is a huge win.  Just to show you
how big, here's the diffstat:

You gotta love it when you remove 5x more code than you add, *and* make it so
much more understandable.

One thing I noticed was lots of skipped tests when running under `make
check_all` or a virtualenv.  As we discussed on IRC, you're going to fix
those, and I'll review what you've got so far.

I'll skip the deletions and any uncontroversial new code.

=== modified file 'gwibber/gwibber/tests/test_protocols.py'
--- gwibber/gwibber/tests/test_protocols.py	2012-09-13 17:50:35 +0000
+++ gwibber/gwibber/tests/test_protocols.py	2012-09-18 20:37:18 +0000
> @@ -27,6 +27,7 @@
>  from gwibber.protocols.twitter import Twitter
>  from gwibber.utils.base import Base
>  from gwibber.utils.manager import ProtocolManager
> +from gwibber.utils.model import SharedModel
>  
>  
>  class TestProtocolManager(unittest.TestCase):
> @@ -126,6 +127,42 @@
>                  thread.join()
>          self.assertEqual(my_protocol.result, 'one:two')
>  
> +    def test_publish(self):
> +        base = Base(None)
> +        with self.assertRaises(TypeError):
> +            base._publish(invalid_argument='not good')

I don't think you need the context manager version of this, since you're not
checking anything about the raised exception.  You could possibly check
details about the exception, e.g. the bad column name (more on this later),
but if not, then I think this would be fine:

self.assertRaises(TypeError, base._publish, invalid_argument='not good')

One other thing we discussed on IRC is that these tests are not well isolated
from the environment.  Because these are not running inside TestDBus, they
actually pollute the user's session.  Other than the problem that running the
test would show bogus data in the Gwibber ui, the other problem is that the
model doesn't get reset after every test, so I bet it persists between test
runs.  Which means it's not clear this is actually testing what you think it's
testing.

The solution is to move all the _publish() tests to TestDBus, since that
ensures it's running on a temporary, isolated session bus, and thus as Ken
says, it should have its own SharedModel.

If TestDBus gets too unwieldy as a result, we'll have to find a way to run
multiple TestCases under the temporary session bus.  (It should be doable, but
maybe not so easily.)

> +
> +        # First message gets through.
> +        self.assertTrue(base._publish(
> +            service='facebook', account_id='1/facebook',
> +            message_id='1234', stream='messages', sender='fred',
> +            sender_nick='freddy', from_me=True, timestamp='today',
> +            message='hello, @jimmy', likes=10, liked=True))
> +
> +        # freddy's duplicate message to Twitter is accepted.
> +        self.assertTrue(base._publish(
> +            service='twitter', account_id='2/twitter',
> +            message_id='5678', stream='messages', sender='fred',
> +            sender_nick='freddy', from_me=True, timestamp='today',
> +            message='hello jimmy', likes=10, liked=False))
> +
> +        # But there is only one message in the model.
> +        self.assertEqual(1, SharedModel.get_n_rows())
> +        self.assertEqual(SharedModel.get_row(SharedModel.get_first_iter())[7],
> +                         'hello, @jimmy')
> +        self.assertEqual(SharedModel.get_row(SharedModel.get_first_iter())[0],
> +            [['facebook', '1/facebook', '1234'],
> +             ['twitter', '2/twitter', '5678']])
> +
> +        # Tedtholomew is allowed to say the same thing in his message though
> +        self.assertTrue(base._publish(
> +            service='facebook', account_id='1/facebook',
> +            message_id='34567', stream='messages', sender='tedtholomew',
> +            sender_nick='teddy', from_me=False, timestamp='today',
> +            message='hello, @jimmy', likes=10, liked=True))
> +
> +        self.assertEqual(2, SharedModel.get_n_rows())
> +

Of course, you know what I'm going to say about these tests. :)

I think these should all be in separate test_*() methods.  Long tests like
this are usually easier to write, but much more difficult to debug when they
fail.  I generally think it's better to have smaller, more isolated tests with
fewer dependencies on earlier results.  Then, when the tests fail later, it's
easier to reason about what could be going wrong.

For example, one test would assert that the data is missing from the table,
then try to _publish() it, asserting a True return value, then assert that the
data existed in the table.

A second test would be very similar, except that it would try to insert the
exact same data, also assert a True return value, but then show that only one
row of the data is in the table.

A possible third test would use the same key as the original row, but
different non-key data.  You'd then show that the second _publish() didn't
overwrite the data of the first.

There are probably variations of the above with separate threads that should
get written, but that will be much trickier, so I think it would be okay to
TODO/XXX them for later.

>      def test_basic_login(self):
>          # Try to log in twice.  The second login attempt returns False because
>          # it's already logged in.

=== modified file 'gwibber/gwibber/utils/base.py'
--- gwibber/gwibber/utils/base.py	2012-09-13 17:50:35 +0000
+++ gwibber/gwibber/utils/base.py	2012-09-18 20:37:18 +0000
> @@ -20,8 +20,25 @@
>      ]
>  
>  
> -from threading import Thread
> -
> +import re
> +import string
> +
> +from threading import Thread, Lock
> +
> +from gwibber.utils.model import SharedModel, COLUMN_NAMES
> +
> +
> +regex_sub = re.compile('[{}\s]'.format(re.escape(string.punctuation))).sub

I think you just impaled some kitten on a rainbow unicorn's horn.

I have no idea what the purpose of this is.  What is this regexp trying to do?
Why is it needed?  My guess is that you want to remove some punctuation from
something, but I have no idea why you'd want to do that.  Plus, one liners
like this are always head-scratchers: "um, oh wait! you're inserting some
escaped characters into a regexp character set with some trailing
whitespace".

> +
> +def strip(message):
> +    """Return a string suitable for fuzzy comparisons."""
> +    message = message.replace('http://', '')
> +    message = message.replace('https://', '')
> +    message = message.replace('gwibber:/', '')
> +    return regex_sub('', message)

This adds to the confusion.  I don't know why "fuzzy comparisons" are
important here, or why the transformations help make the message argument more
suitable for them.

Imagine yourself coming on this code cold, would you understand its purpose?
Pretend you're reverse engineering it all; what explanation would you provide
about what's going on that you wish were there to save you a day of
confusion? :)

I think the purpose of all this is to provide you with a unique key that you
could use to detect duplicate rows.  Am I right?  If so, then maybe also
rename strip() to something more related to its purpose, rather than its
implementation.

> +
> +
> +publish_lock = Lock()

Since this is private to the module, _publish_lock is probably a better name.

>  
>  class Base:
>      class info:
> @@ -53,6 +70,42 @@
>          thread.daemon = True
>          thread.start()
>  
> +    def _publish(self, service, account_id, message_id, **kwargs):

This means that you can never have columns called 'service', 'account_id', or
'message_id', right?  Do you think that will ever be a problem in practice?

The fundamental issue is that you've got a namespace collision.  This is a not
uncommon pattern, and there are various ways out of it.

First, you can try to choose argument names that *really* will never be used
as column names.  An example might be names that start with an underscore
(that's probably a good enough rule to be sure to follow in libgwibber).

Another thing you can do is reduce the number of possibly offending
arguments.  E.g. you could combine those three elements into a 3-tuple
argument such as this:

def _publish(self, _key, **kwargs):
        """...

:param _key: A 3-tuple of the format (service, account_id, message_id)
            which represents the key into a row of the table.  This argument
            is passed as a 3-tuple so as to avoid possible future name clashes
            with column names.
        """
        service, account_id, message_id = _key

> +        """Publish fresh data into the stream, without duplicating anything.
> +
> +        Returns True if the message was appended, or if the message was
> +        already present, returns False if unable to append new messagee.

s/messagee/messages/

> +        """
> +        message = COLUMN_NAMES.index('message')
> +        sender = COLUMN_NAMES.index('sender')
> +        ids = COLUMN_NAMES.index('message_ids')

These are constants, right?  Perhaps you can calculate them in model.py and
just import them here?  Perhaps with that are a little more evocative of what
they are, e.g. MESSAGE_COLUMN or somesuch?  Or, see below for another
suggestion.

> +
> +        args = [''] * len(COLUMN_NAMES)
> +        args[ids] = [[service, account_id, message_id]]

Since this will be confusing to someone when they first read this, you might
refer them to the comment in model.py about why this column is a nested
array.

> +
> +        # The basic idea is that you call _publish() with keyword
> +        # arguments only, making the calling code a bit more explicit,
> +        # and then this converts it from an unordered dict into the
> +        # ordered list required by SharedModel.append().
> +        for column_name, value in kwargs.items():
> +            try:
> +                args[COLUMN_NAMES.index(column_name)] = value

.index() is actually not a cheap call to make, because it actually has to do a
linear search of the sequence every time.  Since this is all constant anyway,
you could calculate one more object in model.py and import it here.  Create a
mapping from column name to index like so:

COLUMN_INDICES = {name: i for i, name in enumerate(COLUMN_NAMES)}

Now, here, and in the code above, all you would need is:

bad_kwargs = []
        for column_name, value in kwargs.items():
            index = COLUMN_INDICES.get(column_name)
            if index is None:
                bad_kwargs.append(column_name)
            else:
                args[index] = value
        if len(bad_kwargs) > 0:
            raise TypeError('Unexpected keyword arguments: {}'.format(
                            COMMASPACE.join(bad_kwargs)))

Why do I do it like that?  So that if more than one bad keyword argument is
given, you'll see them all the first time you hit this code.

I'm going to make one other suggestion about how to do all this, and I'll let
you decide which you like.

args = []
        for column_name, type_code in SCHEMA:
            # This one is special because blah blah blah.
            if column_name == 'message_ids':
                args.append([[service, account_id, message_id]])
            else:
                # If not given, the column's value is an empty string.
                args.append(kwargs.pop(column_name, ''))
        # All keyword arguments must correspond to column names, so any
        # leftover arguments are not allowed.
        if len(kwargs) > 0:
            raise TypeError('Unexpected keyword arguments: {}'.format(
                            COMMASPACE.join(kwargs)))

{}.pop() is kind of nice, isn't it? :)

> +            except ValueError:
> +                raise TypeError(
> +                    "_publish() got an unexpected keyword argument '{}'".format(
> +                        column_name))
> +
> +        with publish_lock:
> +            # Don't let duplicate messages into the model, but do
> +            # record the unique message ids of each duplicate message.
> +            stripped = strip(args[sender] + args[message])
> +            for row in SharedModel:
> +                if stripped == strip(row[sender] + row[message]):
> +                    row[ids] = row[ids] + args[ids]
> +                    return True

This code just confuses the hell out of me.  I had to re-read it several
times, and I'm still not sure why the concatenation occurs.  Is that tested?

Maybe one way to make it more intelligible would be to rename strip() to
unique_key() and pass in the entire row.  The you might see something like:

new_key = unique_key(args)
            for row in SharedModel:
                if new_key == unique_key(row):
                    # There is a row collision.  Discard the remaining data,
                    # but append the new message ids so that the front end can
                    # display all the sources of this data.
                    row[MESSAGE_IDS_INDEX].extend(args[MESSAGE_IDS_INDEX])

Also, is there no more efficient way of asking whether the key already exists
in the database than iterating through all the rows?  Couldn't that get
extremely inefficient?

> +
> +            return bool(SharedModel.append(*args))

What does SharedModel.append() actually return?

> +
>      def _login(self):
>          """Prevent redundant login attempts."""
>          # The first time the user logs in, we expect old_token to be None.

=== added file 'gwibber/gwibber/utils/model.py'
--- gwibber/gwibber/utils/model.py	1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/utils/model.py	2012-09-18 20:37:18 +0000
> @@ -0,0 +1,100 @@
> +# Copyright (C) 2012 Canonical Ltd
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""The Dee.SharedModel interface layer.
> +
> +Dee.SharedModel is comparable to a Gtk.ListStore, except that it
> +shares its state between processes using DBus. When gwibber-service
> +downloads new messages from a website, it inserts those messages into
> +this SharedModel instance, which then triggers a callback in the Vala
> +frontend, which knows to display the new messages there.
> +"""
> +
> +
> +__all__ = [
> +    'SharedModel',
> +    'COLUMN_NAMES',
> +    'COLUMN_TYPES',
> +    ]
> +
> +
> +from gi.repository import Dee
> +
> +
> +# Most of this schema is very straightforward, but the 'service'

I think you mean s/service/message_ids/ right?

> +# column needs a bit of explanation:
> +#
> +# It is a two-dimensional array (ie, an array of arrays). Each inner
> +# array contains three elements: the name of the protocol, the
> +# account_id (like '6/flickr' or '3/facebook'), followed by the
> +# message_id for that particular service.
> +#
> +# Then, there will be one of these triples present for every service
> +# on which the message exists. So for example, if the user posts the
> +# same message to both facebook and twitter, that message will appear
> +# as a single row in this schema, and the 'service' column will look
> +# something like this:
> +#
> +# [
> +#     ['facebook', '2/facebook', '12345'],
> +#     ['twitter', '3/twitter', '987654'],
> +# ]

Could you add an explanation of *why* the inner one is an array?  I vaguely
recall you and Ken discussing that this could be a hash of the concatenated
values.  A comment describing why this design was chosen would be helpful I
think.

> +SCHEMA = (
> +    ('message_ids',    'aas'),
> +    ('stream',         's'),
> +    ('transient',      's'),
> +    ('sender',         's'),
> +    ('sender_nick',    's'),
> +    ('from_me',        'b'),
> +    ('timestamp',      's'),
> +    ('message',        's'),
> +    ('html',           's'),
> +    ('icon_uri',       's'),
> +    ('url',            's'),
> +    ('source',         's'),
> +    ('timestring',     's'),
> +    ('reply_nick',     's'),
> +    ('reply_name',     's'),
> +    ('reply_url',      's'),
> +    ('likes',          'd'),
> +    ('liked',          'b'),
> +    ('retweet_nick',   's'),
> +    ('retweet_name',   's'),
> +    ('retweet_id',     's'),
> +    ('link_picture',   's'),
> +    ('link_name',      's'),
> +    ('link_url',       's'),
> +    ('link_desc',      's'),
> +    ('link_caption',   's'),
> +    ('link_icon',      's'),
> +    ('img_url',        's'),
> +    ('img_src',        's'),
> +    ('img_thumb',      's'),
> +    ('img_name',       's'),
> +    ('video_pic',      's'),
> +    ('video_src',      's'),
> +    ('video_url',      's'),
> +    ('video_name',     's'),
> +    ('comments',       's'),
> +    ('recipient',      's'),
> +    ('recipient_nick', 's'),
> +    ('recipient_icon', 's'),
> +    )
> +
> +
> +COLUMN_NAMES, COLUMN_TYPES = zip(*SCHEMA)
> +
> +
> +SharedModel = Dee.SharedModel.new('com.Gwibber.Streams')
> +SharedModel.set_schema_full(COLUMN_TYPES)

Revision history for this message

Robert Bruce Park (robru) wrote on 2012-09-18:

Download full text (19.7 KiB)

On 12-09-18 05:16 PM, Barry Warsaw wrote:
> First of all, this is fantastic! You've done a great job at removing tons of
> crufty, legacy, mysterious, horrible old code and replaced it with just a
> little bit of good, new, readable code. This is a huge win.

Thanks. I can't take all the credit though... somebody had to write all
this terrible code in the first place! Lots of low-hanging fruit on this
project ;-)

> Just to show you how big, here's the diffstat:
[snip]
> 15 files changed, 231 insertions(+), 1232 deletions(-)

This is actually an old diffstat! The final total was actually 1550
lines deleted ;-)

> You gotta love it when you remove 5x more code than you add, *and* make it so
> much more understandable.

Heh. It was 10x more, but then I expanded the tests... ;-)

> One thing I noticed was lots of skipped tests when running under `make
> check_all` or a virtualenv. As we discussed on IRC, you're going to fix
> those, and I'll review what you've got so far.

Yep, that's already fixed and pushed.

>> + def test_publish(self):
>> + base = Base(None)
>> + with self.assertRaises(TypeError):
>> + base._publish(invalid_argument='not good')
>
> I don't think you need the context manager version of this, since you're not
> checking anything about the raised exception. You could possibly check
> details about the exception, e.g. the bad column name (more on this later),
> but if not, then I think this would be fine:
>
> self.assertRaises(TypeError, base._publish, invalid_argument='not good')

Fair. I had forgotten about your proposed syntax and was thinking that
the with statement was the only way to test for exceptions. You can
change this if you feel strongly about it, but I'm indifferent on this
point.

> One other thing we discussed on IRC is that these tests are not well isolated
> from the environment. Because these are not running inside TestDBus, they
> actually pollute the user's session. Other than the problem that running the
> test would show bogus data in the Gwibber ui, the other problem is that the
> model doesn't get reset after every test, so I bet it persists between test
> runs. Which means it's not clear this is actually testing what you think it's
> testing.

Yes, I know. It's not clear to me exactly how this will fit into
TestDBus though, because SharedModel is not like our other DBus classes
where we are defining the classes ourselves and have a lot of control
over them. I have no idea if SharedModel will respect the custom
sessionbus that TestDBus creates, or how to ensure that that is
happening. I was hoping you'd offer a bit more guidance in this aspect.

Also, on some level we need to understand that we don't need to test
SharedModel itself, but just the code that we use to interface with it.
To that end, we might almost be better off leaving it outside of
TestDBus, and simply create a new SharedModel that uses a different bus
name, the mock that in place of the 'real' sharedmodel. At least as far
as testing Base._publish goes, anyway.

I'll play with this some more tonight I suppose.

> The solution is to move all the _publish() tests to TestDBus, sinc...

On 12-09-18 05:16 PM, Barry Warsaw wrote:
> First of all, this is fantastic!  You've done a great job at removing tons of
> crufty, legacy, mysterious, horrible old code and replaced it with just a
> little bit of good, new, readable code.  This is a huge win.

Thanks. I can't take all the credit though... somebody had to write all 
this terrible code in the first place! Lots of low-hanging fruit on this 
project ;-)

> Just to show you how big, here's the diffstat:
[snip]
>   15 files changed, 231 insertions(+), 1232 deletions(-)

This is actually an old diffstat! The final total was actually 1550 
lines deleted ;-)

> You gotta love it when you remove 5x more code than you add, *and* make it so
> much more understandable.

Heh. It was 10x more, but then I expanded the tests... ;-)

> One thing I noticed was lots of skipped tests when running under `make
> check_all` or a virtualenv.  As we discussed on IRC, you're going to fix
> those, and I'll review what you've got so far.

Yep, that's already fixed and pushed.

>> +    def test_publish(self):
>> +        base = Base(None)
>> +        with self.assertRaises(TypeError):
>> +            base._publish(invalid_argument='not good')
>
> I don't think you need the context manager version of this, since you're not
> checking anything about the raised exception.  You could possibly check
> details about the exception, e.g. the bad column name (more on this later),
> but if not, then I think this would be fine:
>
>         self.assertRaises(TypeError, base._publish, invalid_argument='not good')

Fair. I had forgotten about your proposed syntax and was thinking that 
the with statement was the only way to test for exceptions. You can 
change this if you feel strongly about it, but I'm indifferent on this 
point.

> One other thing we discussed on IRC is that these tests are not well isolated
> from the environment.  Because these are not running inside TestDBus, they
> actually pollute the user's session.  Other than the problem that running the
> test would show bogus data in the Gwibber ui, the other problem is that the
> model doesn't get reset after every test, so I bet it persists between test
> runs.  Which means it's not clear this is actually testing what you think it's
> testing.

Yes, I know. It's not clear to me exactly how this will fit into 
TestDBus though, because SharedModel is not like our other DBus classes 
where we are defining the classes ourselves and have a lot of control 
over them. I have no idea if SharedModel will respect the custom 
sessionbus that TestDBus creates, or how to ensure that that is 
happening. I was hoping you'd offer a bit more guidance in this aspect.

Also, on some level we need to understand that we don't need to test 
SharedModel itself, but just the code that we use to interface with it. 
To that end, we might almost be better off leaving it outside of 
TestDBus, and simply create a new SharedModel that uses a different bus 
name, the mock that in place of the 'real' sharedmodel. At least as far 
as testing Base._publish goes, anyway.

I'll play with this some more tonight I suppose.

> The solution is to move all the _publish() tests to TestDBus, since that
> ensures it's running on a temporary, isolated session bus, and thus as Ken
> says, it should have its own SharedModel.

How does TestDBus set up this custom sessionbus? Is it done in such a 
way that importing SharedModel will force it to use the custom bus and 
not the real bus? Because SharedModel handles it's own dbus logic 
internally, I don't see any way to tell SharedModel 'hey, use this other 
bus instead' unless your TestDBus magic is somehow affecting the dbus 
module itself in a process-global way.

> If TestDBus gets too unwieldy as a result, we'll have to find a way to run
> multiple TestCases under the temporary session bus.  (It should be doable, but
> maybe not so easily.)

TestDBus was decimated by the code removals, so a small addition should 
still result in a net decrease in size. I wouldn't worry about that so 
much at this point.

> I think these should all be in separate test_*() methods.  Long tests like
> this are usually easier to write, but much more difficult to debug when they
> fail.  I generally think it's better to have smaller, more isolated tests with
> fewer dependencies on earlier results.  Then, when the tests fail later, it's
> easier to reason about what could be going wrong.
>
> For example, one test would assert that the data is missing from the table,
> then try to _publish() it, asserting a True return value, then assert that the
> data existed in the table.

You're right, I did forget to write a test that asserts that the model 
starts off empty.

>> +regex_sub = re.compile('[{}\s]'.format(re.escape(string.punctuation))).sub
>
> I think you just impaled some kitten on a rainbow unicorn's horn.

Oh barry! Regex is not our enemy!

> I have no idea what the purpose of this is.  What is this regexp trying to do?

It defines a basic matcher that says 'match all punctuation, and 
whitespace too'.

I was hoping the tests would do a better job of demonstrating what this 
was for. Did you notice that the second 'message' that was rejected as a 
duplicate actually had different punctuation than the first one that was 
accepted? This regex is used in the following function to strip out all 
punctuation and spaces from a string.

> Why is it needed?  My guess is that you want to remove some punctuation from
> something, but I have no idea why you'd want to do that.  Plus, one liners
> like this are always head-scratchers: "um, oh wait! you're inserting some
> escaped characters into a regexp character set with some trailing
> whitespace".
>
>> +
>> +def strip(message):
>> +    """Return a string suitable for fuzzy comparisons."""
>> +    message = message.replace('http://', '')
>> +    message = message.replace('https://', '')
>> +    message = message.replace('gwibber:/', '')
>> +    return regex_sub('', message)
>
> This adds to the confusion.  I don't know why "fuzzy comparisons" are
> important here, or why the transformations help make the message argument more
> suitable for them.

Hey man, this is still better than the code that it replaced. The 
'message.replace' lines I don't fully understand, but they were insisted 
upon by kenvandine, because that's what libgwibber is currently doing.

The idea is that, we don't want duplicate messages to show up in the 
stream of messages that are visible to the user. But different social 
media sites attach different semantic meanings to different punctuation 
marks, so we want to ignore those for the sake of determining whether 
one message is actually identical to another or not. So we need to strip 
them out for the sake of comparing the strings. Consider this example:

Fred uses Gwibber to post identical messages on Twitter and Google+ 
(pretend that we support G+ for a moment). Fred writes "Hey jimbob, been 
to http://example.com lately?", and this message might show up on 
Twitter like "Hey @jimbob, been to example.com lately?", but it might 
show up on G+ like "Hey +jimbob, been to http://example.com lately?". So 
we need to strip out all the possibly-different bits in order to 
identify that these messages are exactly the same. So in both those 
cases above, the string is converted into 
"Heyjimbobbeentoexamplecomlately" and then they compare equally, so 
we've identified a duplicate message.

> Imagine yourself coming on this code cold, would you understand its purpose?
> Pretend you're reverse engineering it all; what explanation would you provide
> about what's going on that you wish were there to save you a day of
> confusion? :)

Ok, so it needs some better comments explaining it, but it had to be 
done with regex for performance reasons. Using a list comprehension to 
build a string was shown to be 3x slower[1], and this function gets 
executed once for each message in the stream (so it is O(n)). Just wait 
until there are a lot of messages present, and then you'll be sorry ;-)

[1] 
http://stackoverflow.com/questions/265960/best-way-to-strip-punctuation-from-a-string-in-python/266162#266162

> I think the purpose of all this is to provide you with a unique key that you
> could use to detect duplicate rows.  Am I right?  If so, then maybe also
> rename strip() to something more related to its purpose, rather than its
> implementation.

You got it exactly right! It's not so hard to read :-P

If you feel it's really that hard to read, we can change it to the list 
comprehension idiom instead (see 'test_set' in the above link), but not 
without doing some kind of cache/memoizer to stop it having to run so 
many times. Actually strip() is a good candidate for memoization because 
it is called repeatedly with the same output, doesn't depend on any 
outside environment other than what is passed to it, and really does 
need to be fast (otherwise there will be big delays trying to add new 
messages to the stream).

>> +publish_lock = Lock()
>
> Since this is private to the module, _publish_lock is probably a better name.

Ok.

>> +    def _publish(self, service, account_id, message_id, **kwargs):
>
> This means that you can never have columns called 'service', 'account_id', or
> 'message_id', right?  Do you think that will ever be a problem in practice?

Nope, I discussed this with kenvandine. He specifically said that 
message_id could never possibly be a column in the schema (and he didn't 
say it explicitly, but his reasoning does extend to the other names, 
too). The reason is that those three things are not unique to each row, 
the first 'aas' column contains this information instead.

> The fundamental issue is that you've got a namespace collision.  This is a not
> uncommon pattern, and there are various ways out of it.

Nope, no namespace collision. Think of these as _required_ schema 
columns. They just couldn't be defined explicitly in the schema because 
'aas' is an inherently vague data type. These three keywords are the 
names of the three strings that live in the 'aas' array-of-arrays.

> First, you can try to choose argument names that *really* will never be used
> as column names.  An example might be names that start with an underscore
> (that's probably a good enough rule to be sure to follow in libgwibber).

They already are column names. They are required column names. It is the 
same namespace :-P

>> +        message = COLUMN_NAMES.index('message')
>> +        sender = COLUMN_NAMES.index('sender')
>> +        ids = COLUMN_NAMES.index('message_ids')
>
> These are constants, right?  Perhaps you can calculate them in model.py and
> just import them here?  Perhaps with that are a little more evocative of what
> they are, e.g. MESSAGE_COLUMN or somesuch?  Or, see below for another
> suggestion.

Oh yeah, good point.

>> +        args = [''] * len(COLUMN_NAMES)
>> +        args[ids] = [[service, account_id, message_id]]
>
> Since this will be confusing to someone when they first read this, you might
> refer them to the comment in model.py about why this column is a nested
> array.

Ok.

>> +        # The basic idea is that you call _publish() with keyword
>> +        # arguments only, making the calling code a bit more explicit,
>> +        # and then this converts it from an unordered dict into the
>> +        # ordered list required by SharedModel.append().
>> +        for column_name, value in kwargs.items():
>> +            try:
>> +                args[COLUMN_NAMES.index(column_name)] = value
>
> .index() is actually not a cheap call to make, because it actually has to do a
> linear search of the sequence every time.  Since this is all constant anyway,
> you could calculate one more object in model.py and import it here.  Create a
> mapping from column name to index like so:
>
> COLUMN_INDICES = {name: i for i, name in enumerate(COLUMN_NAMES)}
>
> Now, here, and in the code above, all you would need is:
>
>          bad_kwargs = []
>          for column_name, value in kwargs.items():
>              index = COLUMN_INDICES.get(column_name)
>              if index is None:
>                  bad_kwargs.append(column_name)
>              else:
>                  args[index] = value
>          if len(bad_kwargs) > 0:
>              raise TypeError('Unexpected keyword arguments: {}'.format(
>                              COMMASPACE.join(bad_kwargs)))
>
> Why do I do it like that?  So that if more than one bad keyword argument is
> given, you'll see them all the first time you hit this code.

Good call.

> I'm going to make one other suggestion about how to do all this, and I'll let
> you decide which you like.
>
>          args = []
>          for column_name, type_code in SCHEMA:
>              # This one is special because blah blah blah.
>              if column_name == 'message_ids':
>                  args.append([[service, account_id, message_id]])
>              else:
>                  # If not given, the column's value is an empty string.
>                  args.append(kwargs.pop(column_name, ''))
>          # All keyword arguments must correspond to column names, so any
>          # leftover arguments are not allowed.
>          if len(kwargs) > 0:
>              raise TypeError('Unexpected keyword arguments: {}'.format(
>                              COMMASPACE.join(kwargs)))
>
> {}.pop() is kind of nice, isn't it? :)

Yeah, dict.pop is fun, but overall I don't like this one. 'if' 
statements that only trigger once inside a loop really bother me, for 
performance reasons. You have to test that test every single time, and 
it's only ever true the first time.

>> +        with publish_lock:
>> +            # Don't let duplicate messages into the model, but do
>> +            # record the unique message ids of each duplicate message.
>> +            stripped = strip(args[sender] + args[message])
>> +            for row in SharedModel:
>> +                if stripped == strip(row[sender] + row[message]):
>> +                    row[ids] = row[ids] + args[ids]
>> +                    return True
>
> This code just confuses the hell out of me.  I had to re-read it several
> times, and I'm still not sure why the concatenation occurs.  Is that tested?

The concatenation joins together the sender's name with his message for 
comparison. If you leave out the sender's name, then you prevent two 
different people from being able to say the same thing. Like Alice 
tweets 'hi' and then Bob tweets 'hi' and Bob's message is lost because 
Alice said it first. By concatenating, you then are comparing 'Alicehi' 
to 'Bobhi' and you see that they are different messages, so they get 
their own rows in the model.

> Maybe one way to make it more intelligible would be to rename strip() to
> unique_key() and pass in the entire row.  The you might see something like:

Nope, you can't pass in the whole row because then the service name 
becomes part of the string, and it no longer is able to realize that 
'Alicehi' on twitter is the same message as 'Alicehi' on facebook, 
because 'twitterAlicehi' is a different string than 'facebookAlicehi'.

It turns out that concatenating the sender's name and message text 
*only* is the correct approach for fuzzy equivalence testing ;-)

>              new_key = unique_key(args)
>              for row in SharedModel:
>                  if new_key == unique_key(row):
>                      # There is a row collision.  Discard the remaining data,
>                      # but append the new message ids so that the front end can
>                      # display all the sources of this data.
>                      row[MESSAGE_IDS_INDEX].extend(args[MESSAGE_IDS_INDEX])
>
> Also, is there no more efficient way of asking whether the key already exists
> in the database than iterating through all the rows?  Couldn't that get
> extremely inefficient?

That's a good question, I'm not sure. The problem is that 
Dee.SharedModel is kind of a custom gobjecty thing, and it's not well 
documented, and I'm not sure how good the python overrides are (it has 
some but seemingly not all of the methods you would expect to be able to 
treat it as a true iterable).

I was hoping that we might just maintain a global set() instance that 
can contain pre-stripped strings, and then we can just test for 
membership there, but then we still need the ability to append to the 
message_ids 'aas' in the event that we are dealing with a duplicate. I 
suppose what we really need then is a global dict() instance, where the 
keys are our stripped strings, and the values are DeeModelIters, such 
that we can get the DeeModelIter back easily and then modify that row 
when we are adding a duplicate message.

>> +            return bool(SharedModel.append(*args))
>
> What does SharedModel.append() actually return?

A DeeModelIter, which can be used to modify the row later. Very much 
like a GtkListStore if you're familiar with those. I'm not actually 
aware of what might cause .append to fail, but it just seemed like the 
smart thing to do to be able to report a failure as False, but then 
again it's not actually clear to me that any other code is going to 
check or care about the return value from Base._publish.

>> +# Most of this schema is very straightforward, but the 'service'
>
> I think you mean s/service/message_ids/ right?

Yep, changed the name and forgot to update the comment.

>> +#     ['facebook', '2/facebook', '12345'],
>> +#     ['twitter', '3/twitter', '987654'],
>> +# ]
>
> Could you add an explanation of *why* the inner one is an array?  I vaguely
> recall you and Ken discussing that this could be a hash of the concatenated
> values.  A comment describing why this design was chosen would be helpful I
> think.

No, you're getting two different conversations confused. the 'aas' thing 
used to be an 'as' from years ago, when 'aas' was buggy/broken in 
python. The string was concatenated with colons, and thus looked like 
'twitter:3/twitter:987654' and then libgwibber had to do a bunch of 
expensive string manipulations on it, stupidly.

ken and I agreed that 'aas' is actually the correct data type for the 
information being contained, the previous 'as' was just an ugly hack 
workaround for a bug that was fixed long ago.

The hashing thing that ken and I were talking about was in regards to 
the strip function above. ken mentioned wanting the comparison done on 
hashes instead of on stripped strings, but then he showed me the 
existing vala code (which doesn't do any hashing) and I didn't see any 
reason to add any hashing to that algorithm, so I didn't bother).

>> +SCHEMA = (
>> +    ('message_ids',    'aas'),
>> +    ('stream',         's'),
>> +    ('transient',      's'),
>> +    ('sender',         's'),
>> +    ('sender_nick',    's'),
>> +    ('from_me',        'b'),
>> +    ('timestamp',      's'),
>> +    ('message',        's'),
>> +    ('html',           's'),
>> +    ('icon_uri',       's'),
>> +    ('url',            's'),
>> +    ('source',         's'),
>> +    ('timestring',     's'),
>> +    ('reply_nick',     's'),
>> +    ('reply_name',     's'),
>> +    ('reply_url',      's'),
>> +    ('likes',          'd'),
>> +    ('liked',          'b'),
>> +    ('retweet_nick',   's'),
>> +    ('retweet_name',   's'),
>> +    ('retweet_id',     's'),
>> +    ('link_picture',   's'),
>> +    ('link_name',      's'),
>> +    ('link_url',       's'),
>> +    ('link_desc',      's'),
>> +    ('link_caption',   's'),
>> +    ('link_icon',      's'),
>> +    ('img_url',        's'),
>> +    ('img_src',        's'),
>> +    ('img_thumb',      's'),
>> +    ('img_name',       's'),
>> +    ('video_pic',      's'),
>> +    ('video_src',      's'),
>> +    ('video_url',      's'),
>> +    ('video_name',     's'),
>> +    ('comments',       's'),
>> +    ('recipient',      's'),
>> +    ('recipient_nick', 's'),
>> +    ('recipient_icon', 's'),
>> +    )
>> +
>> +
>> +COLUMN_NAMES, COLUMN_TYPES = zip(*SCHEMA)
>> +
>> +
>> +SharedModel = Dee.SharedModel.new('com.Gwibber.Streams')
>> +SharedModel.set_schema_full(COLUMN_TYPES)
>
>

Revision history for this message

Robert Bruce Park (robru) wrote on 2012-09-18:

Download full text (19.7 KiB)

Thanks. I can't take all the credit though... somebody had to write all
this terrible code in the first place! Lots of low-hanging fruit on this
project ;-)

> Just to show you how big, here's the diffstat:
[snip]
> 15 files changed, 231 insertions(+), 1232 deletions(-)

This is actually an old diffstat! The final total was actually 1550
lines deleted ;-)

> You gotta love it when you remove 5x more code than you add, *and* make it so
> much more understandable.

Heh. It was 10x more, but then I expanded the tests... ;-)

> One thing I noticed was lots of skipped tests when running under `make
> check_all` or a virtualenv. As we discussed on IRC, you're going to fix
> those, and I'll review what you've got so far.

Yep, that's already fixed and pushed.

I'll play with this some more tonight I suppose.

> The solution is to move all the _publish() tests to TestDBus, sinc...

On 12-09-18 05:16 PM, Barry Warsaw wrote:
> First of all, this is fantastic!  You've done a great job at removing tons of
> crufty, legacy, mysterious, horrible old code and replaced it with just a
> little bit of good, new, readable code.  This is a huge win.

Thanks. I can't take all the credit though... somebody had to write all 
this terrible code in the first place! Lots of low-hanging fruit on this 
project ;-)

> Just to show you how big, here's the diffstat:
[snip]
>   15 files changed, 231 insertions(+), 1232 deletions(-)

This is actually an old diffstat! The final total was actually 1550 
lines deleted ;-)

> You gotta love it when you remove 5x more code than you add, *and* make it so
> much more understandable.

Heh. It was 10x more, but then I expanded the tests... ;-)

> One thing I noticed was lots of skipped tests when running under `make
> check_all` or a virtualenv.  As we discussed on IRC, you're going to fix
> those, and I'll review what you've got so far.

Yep, that's already fixed and pushed.

>> +    def test_publish(self):
>> +        base = Base(None)
>> +        with self.assertRaises(TypeError):
>> +            base._publish(invalid_argument='not good')
>
> I don't think you need the context manager version of this, since you're not
> checking anything about the raised exception.  You could possibly check
> details about the exception, e.g. the bad column name (more on this later),
> but if not, then I think this would be fine:
>
>         self.assertRaises(TypeError, base._publish, invalid_argument='not good')

Fair. I had forgotten about your proposed syntax and was thinking that 
the with statement was the only way to test for exceptions. You can 
change this if you feel strongly about it, but I'm indifferent on this 
point.

> One other thing we discussed on IRC is that these tests are not well isolated
> from the environment.  Because these are not running inside TestDBus, they
> actually pollute the user's session.  Other than the problem that running the
> test would show bogus data in the Gwibber ui, the other problem is that the
> model doesn't get reset after every test, so I bet it persists between test
> runs.  Which means it's not clear this is actually testing what you think it's
> testing.

Yes, I know. It's not clear to me exactly how this will fit into 
TestDBus though, because SharedModel is not like our other DBus classes 
where we are defining the classes ourselves and have a lot of control 
over them. I have no idea if SharedModel will respect the custom 
sessionbus that TestDBus creates, or how to ensure that that is 
happening. I was hoping you'd offer a bit more guidance in this aspect.

Also, on some level we need to understand that we don't need to test 
SharedModel itself, but just the code that we use to interface with it. 
To that end, we might almost be better off leaving it outside of 
TestDBus, and simply create a new SharedModel that uses a different bus 
name, the mock that in place of the 'real' sharedmodel. At least as far 
as testing Base._publish goes, anyway.

I'll play with this some more tonight I suppose.

> The solution is to move all the _publish() tests to TestDBus, since that
> ensures it's running on a temporary, isolated session bus, and thus as Ken
> says, it should have its own SharedModel.

> If TestDBus gets too unwieldy as a result, we'll have to find a way to run
> multiple TestCases under the temporary session bus.  (It should be doable, but
> maybe not so easily.)

TestDBus was decimated by the code removals, so a small addition should 
still result in a net decrease in size. I wouldn't worry about that so 
much at this point.

You're right, I did forget to write a test that asserts that the model 
starts off empty.

>> +regex_sub = re.compile('[{}\s]'.format(re.escape(string.punctuation))).sub
>
> I think you just impaled some kitten on a rainbow unicorn's horn.

Oh barry! Regex is not our enemy!

> I have no idea what the purpose of this is.  What is this regexp trying to do?

It defines a basic matcher that says 'match all punctuation, and 
whitespace too'.

[1] 
http://stackoverflow.com/questions/265960/best-way-to-strip-punctuation-from-a-string-in-python/266162#266162

You got it exactly right! It's not so hard to read :-P

>> +publish_lock = Lock()
>
> Since this is private to the module, _publish_lock is probably a better name.

Ok.

> The fundamental issue is that you've got a namespace collision.  This is a not
> uncommon pattern, and there are various ways out of it.

They already are column names. They are required column names. It is the 
same namespace :-P

Oh yeah, good point.

Ok.

Good call.

> Maybe one way to make it more intelligible would be to rename strip() to
> unique_key() and pass in the entire row.  The you might see something like:

It turns out that concatenating the sender's name and message text 
*only* is the correct approach for fuzzy equivalence testing ;-)

>> +            return bool(SharedModel.append(*args))
>
> What does SharedModel.append() actually return?

>> +# Most of this schema is very straightforward, but the 'service'
>
> I think you mean s/service/message_ids/ right?

Yep, changed the name and forgot to update the comment.

ken and I agreed that 'aas' is actually the correct data type for the 
information being contained, the previous 'as' was just an ugly hack 
workaround for a bug that was fixed long ago.

lp:~robru/gwibber/dee updated on 2012-09-19

1443. By Robert Bruce Park on 2012-09-19

Part one of cleanup based on barry's review.

a) Stop iterating over the SharedModel, because that's stupidly slow
in practise.

b) cache the output of strip(), making the performance of _publish
O(1) instead of O(n)

1444. By Robert Bruce Park on 2012-09-19

Part two of the cleanup based on barry's review.

a) SharedModel is now mocked in the tests so that we are not dumping
test data into the live SharedModel.

b) Each test is broken down into more bitesize units for easier
understanding when something goes wrong.

1445. By Robert Bruce Park on 2012-09-19

Avoid regex in order to improve readability.

Revision history for this message

Barry Warsaw (barry) wrote on 2012-09-19:

Download full text (22.8 KiB)

On Sep 18, 2012, at 11:48 PM, Robert Bruce Park wrote:

>This is actually an old diffstat! The final total was actually 1550
>lines deleted ;-)

Happy happy, joy joy!

>> I don't think you need the context manager version of this, since you're not
>> checking anything about the raised exception. You could possibly check
>> details about the exception, e.g. the bad column name (more on this later),
>> but if not, then I think this would be fine:
>>
>> self.assertRaises(TypeError, base._publish, invalid_argument='not good')
>
>Fair. I had forgotten about your proposed syntax and was thinking that
>the with statement was the only way to test for exceptions. You can
>change this if you feel strongly about it, but I'm indifferent on this
>point.

Right, the context manager is really useful if you want to make additional
assertions about the exception that was raised.

For example, let's say you raised some Gwibber-specific exception, and you
stored the set of invalid arguments on an attribute of that exception. You
could add an assertion that the attribute had the set of strings you expected
it to have, and you'd have to use the context manager syntax to do that. I'm
not suggesting this as a change, just illustrating an example (which I know
you understand, just for anyone else following along).

>> One other thing we discussed on IRC is that these tests are not well
>> isolated from the environment. Because these are not running inside
>> TestDBus, they actually pollute the user's session. Other than the problem
>> that running the test would show bogus data in the Gwibber ui, the other
>> problem is that the model doesn't get reset after every test, so I bet it
>> persists between test runs. Which means it's not clear this is actually
>> testing what you think it's testing.
>
>Yes, I know. It's not clear to me exactly how this will fit into
>TestDBus though, because SharedModel is not like our other DBus classes
>where we are defining the classes ourselves and have a lot of control
>over them. I have no idea if SharedModel will respect the custom
>sessionbus that TestDBus creates, or how to ensure that that is
>happening. I was hoping you'd offer a bit more guidance in this aspect.

I don't know a lot about SharedModel yet, but Ken mentioned on IRC that using
a temporary session bus will isolate the model changes. This should be fairly
easy to verify, though the minimal documentation I've found on Dee doesn't
really say. I'd be more worried about any caching polluting the user's
environment. We'll just have to hack on this to make sure.

>Also, on some level we need to understand that we don't need to test
>SharedModel itself, but just the code that we use to interface with it.
>To that end, we might almost be better off leaving it outside of
>TestDBus, and simply create a new SharedModel that uses a different bus
>name, the mock that in place of the 'real' sharedmodel. At least as far
>as testing Base._publish goes, anyway.

It probably does make sense to have two types of tests, one for outside-dbus
which mocks the SharedModel and one inside-dbus which uses the real shared
model on a temporary bus (which TestDBus...

On Sep 18, 2012, at 11:48 PM, Robert Bruce Park wrote:

>This is actually an old diffstat! The final total was actually 1550 
>lines deleted ;-)

Happy happy, joy joy!

>> I don't think you need the context manager version of this, since you're not
>> checking anything about the raised exception.  You could possibly check
>> details about the exception, e.g. the bad column name (more on this later),
>> but if not, then I think this would be fine:
>>
>>         self.assertRaises(TypeError, base._publish, invalid_argument='not good')
>
>Fair. I had forgotten about your proposed syntax and was thinking that 
>the with statement was the only way to test for exceptions. You can 
>change this if you feel strongly about it, but I'm indifferent on this 
>point.

Right, the context manager is really useful if you want to make additional
assertions about the exception that was raised.

For example, let's say you raised some Gwibber-specific exception, and you
stored the set of invalid arguments on an attribute of that exception.  You
could add an assertion that the attribute had the set of strings you expected
it to have, and you'd have to use the context manager syntax to do that.  I'm
not suggesting this as a change, just illustrating an example (which I know
you understand, just for anyone else following along).

>> One other thing we discussed on IRC is that these tests are not well
>> isolated from the environment.  Because these are not running inside
>> TestDBus, they actually pollute the user's session.  Other than the problem
>> that running the test would show bogus data in the Gwibber ui, the other
>> problem is that the model doesn't get reset after every test, so I bet it
>> persists between test runs.  Which means it's not clear this is actually
>> testing what you think it's testing.
>
>Yes, I know. It's not clear to me exactly how this will fit into 
>TestDBus though, because SharedModel is not like our other DBus classes 
>where we are defining the classes ourselves and have a lot of control 
>over them. I have no idea if SharedModel will respect the custom 
>sessionbus that TestDBus creates, or how to ensure that that is 
>happening. I was hoping you'd offer a bit more guidance in this aspect.

I don't know a lot about SharedModel yet, but Ken mentioned on IRC that using
a temporary session bus will isolate the model changes.  This should be fairly
easy to verify, though the minimal documentation I've found on Dee doesn't
really say.  I'd be more worried about any caching polluting the user's
environment.  We'll just have to hack on this to make sure.

>Also, on some level we need to understand that we don't need to test 
>SharedModel itself, but just the code that we use to interface with it. 
>To that end, we might almost be better off leaving it outside of 
>TestDBus, and simply create a new SharedModel that uses a different bus 
>name, the mock that in place of the 'real' sharedmodel. At least as far 
>as testing Base._publish goes, anyway.

It probably does make sense to have two types of tests, one for outside-dbus
which mocks the SharedModel and one inside-dbus which uses the real shared
model on a temporary bus (which TestDBus already sets up and manages).  The
former tests can be much more extensive than the latter, but there should be
a few really-over-dbus tests of _publish() just to ensure that updates to the
model get expressed in peers of the SharedModel.

It would be more work, but it might also be possible to write the tests once,
and run them both inside and outside dbus.  Probably not worth it though.

>I'll play with this some more tonight I suppose.

Cool.

>> The solution is to move all the _publish() tests to TestDBus, since that
>> ensures it's running on a temporary, isolated session bus, and thus as Ken
>> says, it should have its own SharedModel.
>
>How does TestDBus set up this custom sessionbus?

It does this by running gwibber-service with a specially crafted environment,
set up by the dbus-daemon executable.  If you look in
gwibber/testing/dbus-session.conf.in, you'll see a template for a dbus session
bus configuration file.  The trick here is that this is telling the session
bus to listen on a unix socket in a temporary directory.  The test suite fills
in this tempdir on the fly and then executes dbus-daemon, pointing it to this
config file.  It also puts the com.Gwibber.*.service files in this directory
so that the isolated dbus-daemon will know what executable to run when one of
those services is requested.  Those all point to the gwibber-service
executable in the virtualenv's bin dir, so that ensures that we're testing
against the service we're developing and not the one installed in the system,
and it also explains why the virtualenv must be set up in order for the dbus
tests to not be skipped.  The last piece of the puzzle is that
bin/gwibber-service no longer actually exists; it's created by running
`python3 setup.py install` since it's defined as a console_script entry point.

This also explains why we don't currently test any signals, because the
foreground process, which is running the Python test suite, does *not* connect
to the temporary session bus, it connects to the actual user session bus.
So we would have to either re-exec the test suite under the dbus-daemon regime
described above, or more likely, write a small test script that also ran under
the temporary session dbus to react to those signals.  It's all rather more
complication than I've had time to do, but I think it's important to do at
some point, because those signals are untested, and untested code is broken
code. :)

>Is it done in such a way that importing SharedModel will force it to use the
>custom bus and not the real bus?

Only in the gwibber-service executed by dbus-daemon described above.

>Because SharedModel handles it's own dbus logic internally, I don't see any
>way to tell SharedModel 'hey, use this other bus instead' unless your
>TestDBus magic is somehow affecting the dbus module itself in a
>process-global way.

Yep, it is.  `M-x man RET dbus-daemon RET` will provide additional
information.  Essentially, it's dbus-daemon that's doing all the magic that
the dbus library needs to run gwibber-service on an isolated session bus.

>> If TestDBus gets too unwieldy as a result, we'll have to find a way to run
>> multiple TestCases under the temporary session bus.  (It should be doable,
>> but maybe not so easily.)
>
>TestDBus was decimated by the code removals, so a small addition should 
>still result in a net decrease in size. I wouldn't worry about that so 
>much at this point.

Excellent.

>>> +regex_sub = re.compile('[{}\s]'.format(re.escape(string.punctuation))).sub
>>
>> I think you just impaled some kitten on a rainbow unicorn's horn.
>
>Oh barry! Regex is not our enemy!

For a fun bit of internet lore: http://regex.info/blog/2006-09-15/247

>> I have no idea what the purpose of this is.  What is this regexp trying to
>> do?
>
>It defines a basic matcher that says 'match all punctuation, and 
>whitespace too'.

Right, I get that.  What I'm trying to point out is that it may be difficult
to figure out for another person who is not steeped as deeply in this code as
we are.

A use case: Ubuntu bugs are often fixed by people who are very smart but don't
have deep application domain knowledge about the code they are trying to fix.
The more we make them have to think and reason, the more difficult it will be
for them to fix a bug that is affecting our users.  This isn't to say that
clever or convenient or cryptic code should never be used, but IMHO we should
always keep this drive-by bug-fixer in mind.  I've been that person many times
so I know how painful it can be when you're faced with a critical bug in some
deep code.  If we can help this future Ubuntu bug fixer figure out the code in
half the time, then maybe she can fix two bugs in a day rather than just one.

So when I'm writing or reviewing code, I'm always trying to think about this
person, and trying to anticipate where they will get bogged down.  Sometimes
their life can be made easier by rewriting the code to a more obvious
construct, but of course there are other trade-offs, so sometimes a 5 line
comment explaining both the what and the why of it, will do the trick.

It's difficult for everybody intimately familiar with the code, algorithms,
off-line discussions, whiteboard diagrams, UDS sessions, etc. to step back and
adopt the beginner's mind.  But I do think it's a mindset worth cultivating.

>I was hoping the tests would do a better job of demonstrating what this 
>was for. Did you notice that the second 'message' that was rejected as a 
>duplicate actually had different punctuation than the first one that was 
>accepted?

Actually, no I missed that detail.

This is one reason why I prefer really focused tests.  A test called
test_message_with_different_punctuation_is_a_duplicate() - or some such
reasonable name - would make that much more obvious.

>This regex is used in the following function to strip out all punctuation and
>spaces from a string.

A wonderful comment!

>> Why is it needed?  My guess is that you want to remove some punctuation from
>> something, but I have no idea why you'd want to do that.  Plus, one liners
>> like this are always head-scratchers: "um, oh wait! you're inserting some
>> escaped characters into a regexp character set with some trailing
>> whitespace".

Please note that this part of the review is me with my drive-by Ubuntu bug
fixer hat on.  I'm sorry, I'll try to make that more obvious in the future.
Regexps aren't *necessarily* evil, despite the above link. :)

>>> +
>>> +def strip(message):
>>> +    """Return a string suitable for fuzzy comparisons."""
>>> +    message = message.replace('http://', '')
>>> +    message = message.replace('https://', '')
>>> +    message = message.replace('gwibber:/', '')
>>> +    return regex_sub('', message)
>>
>> This adds to the confusion.  I don't know why "fuzzy comparisons" are
>> important here, or why the transformations help make the message argument more
>> suitable for them.
>
>Hey man, this is still better than the code that it replaced. The 
>'message.replace' lines I don't fully understand, but they were insisted 
>upon by kenvandine, because that's what libgwibber is currently doing.

Right, we just need to make that evident in order to give the drive-byer a
clue as to what's going on, and where possibly to look for future reference.

>The idea is that, we don't want duplicate messages to show up in the 
>stream of messages that are visible to the user. But different social 
>media sites attach different semantic meanings to different punctuation 
>marks, so we want to ignore those for the sake of determining whether 
>one message is actually identical to another or not. So we need to strip 
>them out for the sake of comparing the strings.

Beautiful!  I think I'll just cut and paste this right into a comment. :)

>Consider this example:
>
>Fred uses Gwibber to post identical messages on Twitter and Google+ 
>(pretend that we support G+ for a moment). Fred writes "Hey jimbob, been 
>to http://example.com lately?", and this message might show up on 
>Twitter like "Hey @jimbob, been to example.com lately?", but it might 
>show up on G+ like "Hey +jimbob, been to http://example.com lately?". So 
>we need to strip out all the possibly-different bits in order to 
>identify that these messages are exactly the same. So in both those 
>cases above, the string is converted into 
>"Heyjimbobbeentoexamplecomlately" and then they compare equally, so 
>we've identified a duplicate message.
>
>> Imagine yourself coming on this code cold, would you understand its
>> purpose?  Pretend you're reverse engineering it all; what explanation would
>> you provide about what's going on that you wish were there to save you a
>> day of confusion? :)
>
>Ok, so it needs some better comments explaining it, but it had to be 
>done with regex for performance reasons. Using a list comprehension to 
>build a string was shown to be 3x slower[1], and this function gets 
>executed once for each message in the stream (so it is O(n)). Just wait 
>until there are a lot of messages present, and then you'll be sorry ;-)

>[1] 
>http://stackoverflow.com/questions/265960/best-way-to-strip-punctuation-from-a-string-in-python/266162#266162

Cool.  As I sucked in saying originally, it's not so much the regexp that I'm
concerned with, it's the comprehension of both the what and the why but the
future drive-byer that I want to improve.

>> I think the purpose of all this is to provide you with a unique key that you
>> could use to detect duplicate rows.  Am I right?  If so, then maybe also
>> rename strip() to something more related to its purpose, rather than its
>> implementation.
>
>You got it exactly right! It's not so hard to read :-P
>
>If you feel it's really that hard to read, we can change it to the list 
>comprehension idiom instead (see 'test_set' in the above link), but not 
>without doing some kind of cache/memoizer to stop it having to run so 
>many times. Actually strip() is a good candidate for memoization because 
>it is called repeatedly with the same output, doesn't depend on any 
>outside environment other than what is passed to it, and really does 
>need to be fast (otherwise there will be big delays trying to add new 
>messages to the stream).

We can keep the regexp.

>>> +    def _publish(self, service, account_id, message_id, **kwargs):
>>
>> This means that you can never have columns called 'service', 'account_id',
>> or 'message_id', right?  Do you think that will ever be a problem in
>> practice?
>
>Nope, I discussed this with kenvandine. He specifically said that 
>message_id could never possibly be a column in the schema (and he didn't 
>say it explicitly, but his reasoning does extend to the other names, 
>too). The reason is that those three things are not unique to each row, 
>the first 'aas' column contains this information instead.

Okay.  If Ken says that's the case, then it's safe.

The important thing here to understand is that you cannot call this method
with the explicit keyword arguments given both positionally, and in the
**kwargs.  Python will raise a TypeError in that case.

>> The fundamental issue is that you've got a namespace collision.  This is a
>> not uncommon pattern, and there are various ways out of it.
>
>Nope, no namespace collision. Think of these as _required_ schema 
>columns. They just couldn't be defined explicitly in the schema because 
>'aas' is an inherently vague data type. These three keywords are the 
>names of the three strings that live in the 'aas' array-of-arrays.
>
>> First, you can try to choose argument names that *really* will never be used
>> as column names.  An example might be names that start with an underscore
>> (that's probably a good enough rule to be sure to follow in libgwibber).
>
>They already are column names. They are required column names. It is the 
>same namespace :-P

Sounds great.

>>          bad_kwargs = []
>>          for column_name, value in kwargs.items():
>>              index = COLUMN_INDICES.get(column_name)
>>              if index is None:
>>                  bad_kwargs.append(column_name)
>>              else:
>>                  args[index] = value
>>          if len(bad_kwargs) > 0:
>>              raise TypeError('Unexpected keyword arguments: {}'.format(
>>                              COMMASPACE.join(bad_kwargs)))
>>
>> Why do I do it like that?  So that if more than one bad keyword argument is
>> given, you'll see them all the first time you hit this code.
>
>Good call.
>
>> I'm going to make one other suggestion about how to do all this, and I'll let
>> you decide which you like.
>>
>>          args = []
>>          for column_name, type_code in SCHEMA:
>>              # This one is special because blah blah blah.
>>              if column_name == 'message_ids':
>>                  args.append([[service, account_id, message_id]])
>>              else:
>>                  # If not given, the column's value is an empty string.
>>                  args.append(kwargs.pop(column_name, ''))
>>          # All keyword arguments must correspond to column names, so any
>>          # leftover arguments are not allowed.
>>          if len(kwargs) > 0:
>>              raise TypeError('Unexpected keyword arguments: {}'.format(
>>                              COMMASPACE.join(kwargs)))
>>
>> {}.pop() is kind of nice, isn't it? :)
>
>Yeah, dict.pop is fun, but overall I don't like this one. 'if' 
>statements that only trigger once inside a loop really bother me, for 
>performance reasons. You have to test that test every single time, and 
>it's only ever true the first time.

It doesn't bother me from a performance reason, but aesthetically it *is*
ugly.  I think that's relatively easy to fix:

# Yes, three nested lists because blah blah blah.
        args = [[[service, account_id, message_id]]]
        # Skip message_ids because blah blah blah.
        for column_name, type_code in SCHEMA[1:]:
            # If not given, the column's value is an empty string.
            args.append(kwargs.pop(column_name, ''))
         # All keyword arguments must correspond to column names, so any
         # leftover arguments are not allowed.
         if len(kwargs) > 0:
             raise TypeError('Unexpected keyword arguments: {}'.format(
                             COMMASPACE.join(kwargs)))

Fine with me either way though.

>>> +        with publish_lock:
>>> +            # Don't let duplicate messages into the model, but do
>>> +            # record the unique message ids of each duplicate message.
>>> +            stripped = strip(args[sender] + args[message])
>>> +            for row in SharedModel:
>>> +                if stripped == strip(row[sender] + row[message]):
>>> +                    row[ids] = row[ids] + args[ids]
>>> +                    return True
>>
>> This code just confuses the hell out of me.  I had to re-read it several
>> times, and I'm still not sure why the concatenation occurs.  Is that tested?
>
>The concatenation joins together the sender's name with his message for 
>comparison. If you leave out the sender's name, then you prevent two 
>different people from being able to say the same thing. Like Alice 
>tweets 'hi' and then Bob tweets 'hi' and Bob's message is lost because 
>Alice said it first. By concatenating, you then are comparing 'Alicehi' 
>to 'Bobhi' and you see that they are different messages, so they get 
>their own rows in the model.

Another excellent explanation I might just cut-n-paste into the code. :)

>> Maybe one way to make it more intelligible would be to rename strip() to
>> unique_key() and pass in the entire row.  The you might see something like:
>
>Nope, you can't pass in the whole row because then the service name 
>becomes part of the string, and it no longer is able to realize that 
>'Alicehi' on twitter is the same message as 'Alicehi' on facebook, 
>because 'twitterAlicehi' is a different string than 'facebookAlicehi'.
>
>It turns out that concatenating the sender's name and message text 
>*only* is the correct approach for fuzzy equivalence testing ;-)

No, because it's just that strip() or whatever would pull out row[sender] and
row[message] and do the concatenation itself.  It just moves that logic into
the method that calculates the unique key from the row data.

>> Also, is there no more efficient way of asking whether the key already
>> exists in the database than iterating through all the rows?  Couldn't that
>> get extremely inefficient?
>
>That's a good question, I'm not sure. The problem is that 
>Dee.SharedModel is kind of a custom gobjecty thing, and it's not well 
>documented, and I'm not sure how good the python overrides are (it has 
>some but seemingly not all of the methods you would expect to be able to 
>treat it as a true iterable).

The best I could find is this:

http://developer.ubuntu.com/api/ubuntu-12.04/python/Dee-1.0.html

SharedModel implements this:

http://developer.ubuntu.com/api/ubuntu-12.04/python/Dee-1.0.html#Dee.Model

and yeah, the documentation is, um, sparse.  So it's probably worth playing
around with this to see if there's a better way to find the row.
find_row_sorted() looks promising, as does Dee.Index.lookup().  You gotta
figure there's an efficient way to query the table.

>I was hoping that we might just maintain a global set() instance that 
>can contain pre-stripped strings, and then we can just test for 
>membership there, but then we still need the ability to append to the 
>message_ids 'aas' in the event that we are dealing with a duplicate. I 
>suppose what we really need then is a global dict() instance, where the 
>keys are our stripped strings, and the values are DeeModelIters, such 
>that we can get the DeeModelIter back easily and then modify that row 
>when we are adding a duplicate message.
>
>>> +            return bool(SharedModel.append(*args))
>>
>> What does SharedModel.append() actually return?
>
>A DeeModelIter, which can be used to modify the row later. Very much 
>like a GtkListStore if you're familiar with those. I'm not actually 
>aware of what might cause .append to fail, but it just seemed like the 
>smart thing to do to be able to report a failure as False, but then 
>again it's not actually clear to me that any other code is going to 
>check or care about the return value from Base._publish.

I guess my question is, what is the truth value of a DeeModelIter?  There's no
documentation on it, or whether .append() can return None or something else if
it fails.  Either we'll have to do some experiments to find out what it does
in practice, or maybe just kill the return value.

>>> +# Most of this schema is very straightforward, but the 'service'
>>
>> I think you mean s/service/message_ids/ right?
>
>Yep, changed the name and forgot to update the comment.
>
>>> +#     ['facebook', '2/facebook', '12345'],
>>> +#     ['twitter', '3/twitter', '987654'],
>>> +# ]
>>
>> Could you add an explanation of *why* the inner one is an array?  I vaguely
>> recall you and Ken discussing that this could be a hash of the concatenated
>> values.  A comment describing why this design was chosen would be helpful I
>> think.
>
>No, you're getting two different conversations confused. the 'aas' thing 
>used to be an 'as' from years ago, when 'aas' was buggy/broken in 
>python. The string was concatenated with colons, and thus looked like 
>'twitter:3/twitter:987654' and then libgwibber had to do a bunch of 
>expensive string manipulations on it, stupidly.
>
>ken and I agreed that 'aas' is actually the correct data type for the 
>information being contained, the previous 'as' was just an ugly hack 
>workaround for a bug that was fixed long ago.

Excellent.

>The hashing thing that ken and I were talking about was in regards to 
>the strip function above. ken mentioned wanting the comparison done on 
>hashes instead of on stripped strings, but then he showed me the 
>existing vala code (which doesn't do any hashing) and I didn't see any 
>reason to add any hashing to that algorithm, so I didn't bother).

Cool.  It serves as an opaque string either way.

Revision history for this message

Robert Bruce Park (robru) wrote on 2012-09-19:

Download full text (5.3 KiB)

On 12-09-19 10:52 AM, Barry Warsaw wrote:
>> Yes, I know. It's not clear to me exactly how this will fit into
>> TestDBus though, because SharedModel is not like our other DBus classes
>> where we are defining the classes ourselves and have a lot of control
>> over them. I have no idea if SharedModel will respect the custom
>> sessionbus that TestDBus creates, or how to ensure that that is
>> happening. I was hoping you'd offer a bit more guidance in this aspect.
>
> I don't know a lot about SharedModel yet, but Ken mentioned on IRC that using
> a temporary session bus will isolate the model changes. This should be fairly
> easy to verify, though the minimal documentation I've found on Dee doesn't
> really say. I'd be more worried about any caching polluting the user's
> environment. We'll just have to hack on this to make sure.

The testsuite, as it currently stands in the mp, does not pollute the
users environment. The tests that test Base._publish are using a mocked
SharedModel using a different bus name, and there's even a test that
confirms the user's environ is not polluted.

The other test, that tests SharedModel more directly, does operate on
the user's live environment, but it's a read-only test that doesn't do
any polluting. It basically just tests that the right schema is in
place, which is probably good enough, because anything more than that
would become a test against Dee, rather than a test against Gwibber.

>> Is it done in such a way that importing SharedModel will force it to use the
>> custom bus and not the real bus?
>
> Only in the gwibber-service executed by dbus-daemon described above.
>
>> Because SharedModel handles it's own dbus logic internally, I don't see any
>> way to tell SharedModel 'hey, use this other bus instead' unless your
>> TestDBus magic is somehow affecting the dbus module itself in a
>> process-global way.
>
> Yep, it is. `M-x man RET dbus-daemon RET` will provide additional
> information. Essentially, it's dbus-daemon that's doing all the magic that
> the dbus library needs to run gwibber-service on an isolated session bus.

Ok, so importing our SharedModel instance in main.py would result in an
isolated, testable SharedModel. good to know.

> We can keep the regexp.

Funny, because I actually did replace it with a string comprehension,
and the readability did improve incredibly. My performance concerns were
mitigated by a) no longer iterating over the sharedmodel, calling
strip() each time, and b) caching the results of strip(), such that it's
only necessary to call it once for each new message addition. That
effectively upgrades it from O(n) to O(1), which means the slower
implementation won't really be noticable.

>> It turns out that concatenating the sender's name and message text
>> *only* is the correct approach for fuzzy equivalence testing ;-)
>
> No, because it's just that strip() or whatever would pull out row[sender] and
> row[message] and do the concatenation itself. It just moves that logic into
> the method that calculates the unique key from the row data.

Oh, now i see. You can make that change if you insist ;-)

>>> Also, is there no more efficient way of asking ...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barry Warsaw

Robert Bruce Park

 === modified file 'gwibber/gwibber/main.py'
 --- gwibber/gwibber/main.py	2012-09-13 18:04:03 +0000
 +++ gwibber/gwibber/main.py	2012-09-19 17:22:19 +0000
@@ -31,10 +31,7 @@
  from gwibber.service.connection import ConnectionMonitor
  from gwibber.service.dispatcher import Dispatcher
--from gwibber.service.messages import MessageManager
--from gwibber.service.searches import SearchManager
  from gwibber.service.shortener import URLShorten
--from gwibber.service.streams import StreamManager
  from gwibber.testing.service import TestService
  from gwibber.utils.logging import initialize
  from gwibber.utils.menus import MenuManager
@@ -66,11 +63,7 @@
                 debug=args.debug or gsettings.get_boolean('debug'))
      log = logging.getLogger('gwibber.service')
      log.info('Gwibber backend service starting')
--    # Where is the SQLite database?
--    sqlite_file = (DEFAULT_SQLITE_FILE
--                   if args.database is None
--                   else args.database)
--    database = sqlite3.connect(sqlite_file)
++
      # Set up the DBus main loop.
      DBusGMainLoop(set_as_default=True)
      loop = GLib.MainLoop()
@@ -82,15 +75,12 @@
      # about unused local variables.
      class services:
          connection = ConnectionMonitor()
--        streams = StreamManager(database)
--        dispatcher = Dispatcher(loop, streams, refresh_interval)
++        dispatcher = Dispatcher(loop, refresh_interval)
          menus = MenuManager(dispatcher.Refresh, loop.quit)
--        messages = MessageManager(database)
--        searches = SearchManager(database)
          shorten = URLShorten(gsettings)
      if args.test:
--        services.test = TestService(database, services.connection)
++        services.test = TestService(services.connection)
      try:
          log.info('Starting gwibber-service main loop')
          loop.run()
 === modified file 'gwibber/gwibber/service/dispatcher.py'
 --- gwibber/gwibber/service/dispatcher.py	2012-09-13 18:04:03 +0000
 +++ gwibber/gwibber/service/dispatcher.py	2012-09-19 17:22:19 +0000
@@ -40,13 +40,13 @@
      """This is the primary handler of dbus method calls."""
      __dbus_object_path__ = '/com/gwibber/Service'
--    def __init__(self, mainloop, streams, interval):
++    def __init__(self, mainloop, interval):
          self.bus = dbus.SessionBus()
          bus_name = dbus.service.BusName('com.Gwibber.Service', bus=self.bus)
          super().__init__(bus_name, self.__dbus_object_path__)
          self.mainloop = mainloop
          self._interval = interval
--        self.account_manager = AccountManager(self.Refresh, streams)
++        self.account_manager = AccountManager(self.Refresh)
          self._timer_id = None
          signaler.add_signal('ConnectionOnline', self._on_connection_online)
          signaler.add_signal('ConnectionOffline', self._on_connection_offline)
 === removed file 'gwibber/gwibber/service/messages.py'
 --- gwibber/gwibber/service/messages.py	2012-08-21 14:51:01 +0000
 +++ gwibber/gwibber/service/messages.py	1970-01-01 00:00:00 +0000
@@ -1,47 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""DBus service object for message manager."""
--
--__all__ = [
--    'MessageManager',
--    ]
--
--
--import dbus
--import dbus.service
--
--from gwibber.storage.messages import MessageStorage
--
--
--class MessageManager(dbus.service.Object):
--    __dbus_object_path__ = '/com/gwibber/Messages'
--
--    def __init__(self, db):
--        self.bus = dbus.SessionBus()
--        bus_name = dbus.service.BusName('com.Gwibber.Messages', bus=self.bus)
--        super().__init__(bus_name, self.__dbus_object_path__)
--        self.db = db
--        # Initialize the table.
--        MessageStorage(self.db)
--
--    @dbus.service.signal('com.Gwibber.Messages', signature='ss')
--    def Message(self, change, data):
--        pass
--
--    @dbus.service.method('com.Gwibber.Messages',
--                         in_signature='s',
--                         out_signature='s')
--    def Get(self, id):
--        return MessageStorage(self.db).get_data(id)
 === removed file 'gwibber/gwibber/service/searches.py'
 --- gwibber/gwibber/service/searches.py	2012-08-21 22:42:24 +0000
 +++ gwibber/gwibber/service/searches.py	1970-01-01 00:00:00 +0000
@@ -1,87 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""DBus service object for searches."""
--
--__all__ = [
--    'SearchManager',
--    ]
--
--
--import json
--import uuid
--import logging
--
--import dbus
--import dbus.service
--
--from gwibber.storage.messages import MessageStorage
--from gwibber.storage.searches import SearchStorage
--
--
--log = logging.getLogger('gwibber.service')
--
--
--class SearchManager(dbus.service.Object):
--    __dbus_object_path__ = '/com/gwibber/Searches'
--
--    def __init__(self, db):
--        self.bus = dbus.SessionBus()
--        bus_name = dbus.service.BusName('com.Gwibber.Searches', bus=self.bus)
--        super().__init__(bus_name, self.__dbus_object_path__)
--        self.db = db
--        # Initialize the table.
--        SearchStorage(self.db)
--
--    @dbus.service.signal('com.Gwibber.Searches', signature='s')
--    def Updated(self, data):
--        log.debug('Search Changed: {}'.format(data))
--
--    @dbus.service.signal('com.Gwibber.Searches', signature='s')
--    def Deleted(self, data):
--        log.debug('Search Deleted: {}'.format(data))
--
--    @dbus.service.signal('com.Gwibber.Searches', signature='s')
--    def Created(self, data):
--        log.debug('Search Created: {}'.format(data))
--
--    @dbus.service.method('com.Gwibber.Searches',
--                         in_signature='s',
--                         out_signature='s')
--    def Get(self, id):
--        return SearchStorage(self.db).get_data(id)
--
--    @dbus.service.method('com.Gwibber.Searches', out_signature='s')
--    def List(self):
--        return SearchStorage(self.db).list_all_data()
--
--    @dbus.service.method('com.Gwibber.Searches',
--                         in_signature='s',
--                         out_signature='s')
--    def Create(self, search):
--        data = json.loads(search)
--        id = data['id'] = uuid.uuid1().hex
--        encoded = json.dumps(data)
--        SearchStorage(self.db).add(**data)
--        self.Created(encoded)
--        return id
--
--    @dbus.service.method('com.Gwibber.Searches', in_signature='s')
--    def Delete(self, id):
--        data = self.Get(id)
--        if data:
--            with self.db:
--                SearchStorage(self.db).remove(id)
--                MessageStorage(self.db).remove_transient(id)
--            self.Deleted(data)
 === removed file 'gwibber/gwibber/service/streams.py'
 --- gwibber/gwibber/service/streams.py	2012-08-17 22:30:54 +0000
 +++ gwibber/gwibber/service/streams.py	1970-01-01 00:00:00 +0000
@@ -1,93 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""DBus service object for the streams manager."""
--
--__all__ = [
--    'StreamManager',
--    ]
--
--
--import json
--import uuid
--import logging
--
--import dbus
--import dbus.service
--
--from gwibber.storage.messages import MessageStorage
--from gwibber.storage.streams import StreamStorage
--
--
--log = logging.getLogger('gwibber.service')
--
--
--class StreamManager(dbus.service.Object):
--    __dbus_object_path__ = '/com/gwibber/Streams'
--
--    def __init__(self, db):
--        self.bus = dbus.SessionBus()
--        bus_name = dbus.service.BusName('com.Gwibber.Streams', bus=self.bus)
--        super().__init__(bus_name, self.__dbus_object_path__)
--        self.db = db
--        # Initialize the table.
--        StreamStorage(self.db)
--
--    @dbus.service.signal('com.Gwibber.Streams', signature='s')
--    def Updated(self, data):
--        log.debug('Stream Changed: {}'.format(data))
--
--    @dbus.service.signal('com.Gwibber.Streams', signature='s')
--    def Deleted(self, data):
--        log.debug('Stream Deleted: {}'.format(data))
--
--    @dbus.service.signal('com.Gwibber.Streams', signature='s')
--    def Created(self, data):
--        log.debug('Stream Created: {}'.format(data))
--
--    @dbus.service.method('com.Gwibber.Streams',
--                         in_signature='s',
--                         out_signature='s')
--    def Get(self, id):
--        return StreamStorage(self.db).get_data(id)
--
--    @dbus.service.method('com.Gwibber.Streams', out_signature='s')
--    def List(self):
--        return StreamStorage(self.db).list_all_data()
--
--    @dbus.service.method('com.Gwibber.Streams',
--                         in_signature='ssdssssd',
--                         out_signature='s')
--    def Messages(self, stream, account, time, transient, recipient,
--                 orderby, order, limit):
--        return MessageStorage(self.db).get_messages(
--            stream, account, time, transient, recipient, orderby, order, limit)
--
--    @dbus.service.method('com.Gwibber.Streams',
--                         in_signature='s',
--                         out_signature='s')
--    def Create(self, search):
--        data = json.loads(search)
--        id = data['id'] = uuid.uuid1().hex
--        encoded = json.dumps(data)
--        StreamStorage(self.db).add(**data)
--        self.Created(encoded)
--        return id
--
--    @dbus.service.method('com.Gwibber.Streams', in_signature='s')
--    def Delete(self, id):
--        data = self.Get(id)
--        if data:
--            StreamStorage(self.db).remove(id)
--            self.Deleted(data)
 === removed directory 'gwibber/gwibber/storage'
 === removed file 'gwibber/gwibber/storage/__init__.py'
 === removed file 'gwibber/gwibber/storage/messages.py'
 --- gwibber/gwibber/storage/messages.py	2012-08-21 22:42:24 +0000
 +++ gwibber/gwibber/storage/messages.py	1970-01-01 00:00:00 +0000
@@ -1,172 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""Message storage."""
--
--__all__ = [
--    'MessageStorage',
--    ]
--
--
--import logging
--
--log = logging.getLogger('gwibber.service')
--
--
--COMMASPACE = ', '
--MAX_COUNT = 2000
--
--
--class MessageStorage:
--    """Message storage."""
--
--    def __init__(self, db):
--        self.db = db
--        # Create the table if it does not yet exist.
--        if not self.db.execute('PRAGMA table_info(messages)').fetchall():
--            with self.db:
--                self.db.execute("""
--                CREATE TABLE messages (
--                    id text,
--                    mid text,
--                    account text,
--                    service text,
--                    operation text,
--                    transient text,
--                    stream text,
--                    time integer,
--                    text text,
--                    from_me integer,
--                    to_me integer,
--                    sender text,
--                    recipient text,
--                    data text)
--                """)
--                self.db.execute("""
--                CREATE UNIQUE INDEX idx1 ON messages (
--                    mid, account, operation, transient)
--                """)
--                self.db.execute("""
--                CREATE INDEX IF NOT EXISTS idx2 on messages (
--                    stream, time, transient)
--                """)
--
--    def get_data(self, id):
--        results = self.db.execute(
--            'SELECT data FROM messages WHERE id = ?', (id,)).fetchone()
--        if results:
--            return results[0]
--        return ''
--
--    def get_messages(self, stream, account, time, transient, recipient,
--                     orderby, order, limit):
--        # The home stream is an alias for all streams in all accounts.
--        if stream == 'home':
--            stream = 'all'
--            account = 'all'
--
--        with self.db:
--            if stream == 'sent' and account != 'all':
--                # Get all messages I've sent from a specific account.
--                where = """
--                WHERE from_me = 1 AND account = ?
--                    AND time >= ? AND transient = ?
--                """
--                args = account, time, transient
--            elif stream == 'sent' and account == 'all':
--                # Get all messages I've sent from any account.
--                 where = 'WHERE from_me = 1 AND time >= ? AND transient = ?'
--                 args = time, transient
--            elif transient != '0' and account != 'all' and recipient != '0':
--                where = """
--                WHERE time >= ? AND transient = ?
--                    OR (time >= ? AND account = ? AND recipient = ?)
--                """
--                args = time, transient, time, account, recipient
--            elif stream != 'all' and account != 'all':
--                where = """
--                WHERE stream = ? AND account = ? AND time >= ?
--                    AND transient = ?
--                """
--                args = stream, account, time, transient
--            elif stream == 'all' and account != 'all':
--                where = 'WHERE account = ? AND time >= ? AND transient = ?'
--                args = account, time, transient
--            elif stream != 'all' and account == 'all':
--                where = 'WHERE stream = ? AND time >= ? AND transient = ?'
--                args = stream, time, transient
--            else:
--                where = 'WHERE time >= ? AND transient = ?'
--                args = time, transient
--            query = 'SELECT data FROM messages '
--            query += where
--            query += ' ORDER BY {} {}'.format(orderby, order)
--            if limit > 0:
--                query += ' LIMIT {:d}'.format(limit)
--            results = self.db.execute(query, args)
--        # Collating happens outside of the database transaction.
--        return '[{}]'.format(COMMASPACE.join(row[0] for row in results))
--
--    def maintenance(self, account_manager):
--        log.info('DB maintenance: cleaning up messages table...')
--        accounts = self.db.execute(
--            'SELECT distinct(account) FROM messages').fetchall()
--        for account in accounts:
--            # Adjust for the query returning a tuple.
--            account = account[0]
--            # Does the account exist?
--            global_id, slash, rest = account.partition('/')
--            if slash == '':
--                # There is no global id, thus the account does not exist.
--                exists = False
--            else:
--                exists = account_manager.get_account(global_id)
--            if not exists:
--                log.info('DB maintenance: Removing data for '
--                         'unknown account: {}'.format(account))
--                self.db.execute('DELETE FROM messages WHERE account = ?',
--                                (account,))
--            count = self.db.execute("""
--            SELECT count(data) FROM messages
--            WHERE operation = 'receive'
--              AND stream = 'messages'
--              AND account = ?
--              AND time != 0
--            """, (account,)).fetchone()[0]
--            log.info('DB maintenance: found {} records in messages '
--                     'stream for account {}'.format(count, account))
--            log.info('DB maintenance: purging old data for {}'.format(
--                account))
--            delete_limit = count - MAX_COUNT
--            if delete_limit > 0:
--                self.db.execute("""
--                DELETE FROM messages
--                WHERE account = ?
--                  AND operation = 'receive'
--                  AND stream = 'messages'
--                  AND time IN (
--                        SELECT CAST (time AS int) FROM (
--                            SELECT time FROM messages
--                             WHERE account = ?
--                               AND operation = 'receive'
--                               AND stream = 'messages'
--                               AND time != 0 ORDER BY time ASC LIMIT ?)
--                            ORDER BY time ASC)
--                """, (account, account, delete_limit))
--        self.db.execute('VACUUM')
--
--    def remove_transient(self, transient_id):
--        with self.db:
--            self.db.execute('DELETE FROM messages WHERE transient = ?',
--                            (transient_id,))
 === removed file 'gwibber/gwibber/storage/searches.py'
 --- gwibber/gwibber/storage/searches.py	2012-08-21 21:35:08 +0000
 +++ gwibber/gwibber/storage/searches.py	1970-01-01 00:00:00 +0000
@@ -1,63 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""Search storage."""
--
--__all__ = [
--    'SearchStorage',
--    ]
--
--
--COMMASPACE = ', '
--
--
--class SearchStorage:
--    """Search storage."""
--
--    def __init__(self, db):
--        self.db = db
--        # Create the table if it does not yet exist.
--        if not self.db.execute('PRAGMA table_info(searches)').fetchall():
--            with self.db:
--                self.db.execute("""
--                CREATE TABLE searches (
--                    id text,
--                    name text,
--                    query text,
--                    data text)
--                """)
--
--    def get_data(self, id):
--        """Get the row's data."""
--        results = self.db.execute('SELECT data FROM searches WHERE id = ?',
--                                  (id,)).fetchone()
--        if results:
--            return results[0]
--        return ''
--
--    def list_all_data(self):
--        """Get a string listing all data in the table."""
--        results = self.db.execute('SELECT data FROM searches')
--        return '[{}]'.format(COMMASPACE.join(row[0] for row in results))
--
--    def add(self, id, name, query, data):
--        """Add a row to the table."""
--        with self.db:
--            self.db.execute('INSERT INTO searches VALUES (?, ?, ?, ?)',
--                            (id, name, query, data))
--
--    def remove(self, id):
--        """Remove a row from the table."""
--        with self.db:
--            self.db.execute('DELETE FROM searches WHERE id = ?', (id,))
 === removed file 'gwibber/gwibber/storage/streams.py'
 --- gwibber/gwibber/storage/streams.py	2012-08-17 22:30:54 +0000
 +++ gwibber/gwibber/storage/streams.py	1970-01-01 00:00:00 +0000
@@ -1,68 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""Streams storage."""
--
--__all__ = [
--    'StreamStorage',
--    ]
--
--
--COMMASPACE = ', '
--
--
--class StreamStorage:
--    """Stream storage."""
--
--    def __init__(self, db):
--        self.db = db
--        # Create the table if it does not yet exist.
--        if not self.db.execute('PRAGMA table_info(streams)').fetchall():
--            with self.db:
--                self.db.execute("""
--                CREATE TABLE streams (
--                    id text,
--                    name text,
--                    account text,
--                    operation text,
--                    data text)
--                """)
--
--    def get_data(self, id):
--        """Get the row's data."""
--        with self.db:
--            results = self.db.execute(
--                'SELECT data FROM streams WHERE id = ?',
--                (id,)).fetchone()
--            if results:
--                return results[0]
--            return ''
--
--    def list_all_data(self):
--        """Get a string listing all data in the table."""
--        with self.db:
--            results = self.db.execute('SELECT data FROM streams')
--            return '[{}]'.format(
--                COMMASPACE.join(row[0] for row in results.fetchall()))
--
--    def add(self, id, name, account, operation, data):
--        """Add a row to the table."""
--        with self.db:
--            self.db.execute('INSERT INTO streams VALUES (?, ?, ?, ?, ?)',
--                            (id, name, account, operation, data))
--
--    def remove(self, id):
--        """Remove a row from the table."""
--        with self.db:
--            self.db.execute('DELETE FROM streams WHERE id = ?', (id,))
 === removed file 'gwibber/gwibber/testing/com.Gwibber.Messages.service.in'
 --- gwibber/gwibber/testing/com.Gwibber.Messages.service.in	2012-08-21 14:51:01 +0000
 +++ gwibber/gwibber/testing/com.Gwibber.Messages.service.in	1970-01-01 00:00:00 +0000
@@ -1,3 +0,0 @@
--[D-BUS Service]
--Name=com.Gwibber.Messages
--Exec={BINDIR}/gwibber-service --database {TMPDIR}/gwibber.db --test
 === removed file 'gwibber/gwibber/testing/com.Gwibber.Searches.service.in'
 --- gwibber/gwibber/testing/com.Gwibber.Searches.service.in	2012-08-21 21:05:20 +0000
 +++ gwibber/gwibber/testing/com.Gwibber.Searches.service.in	1970-01-01 00:00:00 +0000
@@ -1,3 +0,0 @@
--[D-BUS Service]
--Name=com.Gwibber.Searches
--Exec={BINDIR}/gwibber-service --database {TMPDIR}/gwibber.db --test
 === removed file 'gwibber/gwibber/testing/com.Gwibber.Streams.service.in'
 --- gwibber/gwibber/testing/com.Gwibber.Streams.service.in	2012-08-21 14:51:01 +0000
 +++ gwibber/gwibber/testing/com.Gwibber.Streams.service.in	1970-01-01 00:00:00 +0000
@@ -1,3 +0,0 @@
--[D-BUS Service]
--Name=com.Gwibber.Streams
--Exec={BINDIR}/gwibber-service --database {TMPDIR}/gwibber.db --test
 === modified file 'gwibber/gwibber/testing/dbus.py'
 --- gwibber/gwibber/testing/dbus.py	2012-09-11 22:32:44 +0000
 +++ gwibber/gwibber/testing/dbus.py	2012-09-19 17:22:19 +0000
@@ -49,10 +49,7 @@
  SERVICES = [
      'com.Gwibber.Connection',
--    'com.Gwibber.Messages',
--    'com.Gwibber.Searches',
      'com.Gwibber.Service',
--    'com.Gwibber.Streams',
      'com.Gwibber.Test',
      'com.Gwibber.URLShorten',
+     ]
 === modified file 'gwibber/gwibber/testing/service.py'
 --- gwibber/gwibber/testing/service.py	2012-09-07 15:20:41 +0000
 +++ gwibber/gwibber/testing/service.py	2012-09-19 17:22:19 +0000
@@ -35,11 +35,10 @@
  class TestService(dbus.service.Object):
      __dbus_object_path__ = '/com/gwibber/Test'
--    def __init__(self, db, monitor):
++    def __init__(self, monitor):
          self.bus = dbus.SessionBus()
          bus_name = dbus.service.BusName('com.Gwibber.Test', bus=self.bus)
          super().__init__(bus_name, self.__dbus_object_path__)
--        self.db = db
          self.id = 0
          self.connection_monitor = monitor
          # Connect signals
@@ -51,52 +50,6 @@
          signaler.add_signal('ConnectionOnline', online_callback)
          signaler.add_signal('ConnectionOffline', offline_callback)
--    @dbus.service.method('com.Gwibber.Test',
--                         in_signature='s',
--                         out_signature='s')
--    def InsertDataIntoMessages(self, data):
--        # Initialize the database if not already created.
--        MessageStorage(self.db)
--        stored_id = str(self.id)
--        self.db.execute("""
--        INSERT INTO messages VALUES
--        (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--        """, (stored_id, stored_id, '', '', '', '', '', 0, '', 0, 0, '', '',
--              data))
--        self.id += 1
--        return stored_id
--
--    @dbus.service.method('com.Gwibber.Test',
--                         in_signature='sss',
--                         out_signature='s')
--    def InsertSearchTransientData(self, name, query, data):
--        # Set things up to test the com.Gwibber.Searches.Delete() method.
--        # Start by initializing the tables.
--        MessageStorage(self.db)
--        SearchStorage(self.db)
--        stored_id = str(self.id)
--        self.db.execute("""
--        INSERT INTO searches VALUES (?, ?, ?, ?)
--        """, (stored_id, name, query, data))
--        # The key thing about adding a row to messages is that the
--        # messages.transient column must be equal to the searches.id column.
--        # It's kind of like a foreign reference.
--        self.db.execute("""
--        INSERT INTO messages VALUES
--        (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--        """, (stored_id, stored_id, '', '', '',
--              stored_id, '', 0, '', 0, 0, '', '', data))
--        self.id += 1
--        return stored_id
--
--    @dbus.service.method('com.Gwibber.Test', in_signature='s')
--    def DeleteDataFromMessages(self, id):
--        # Initialize the database if not already created.
--        MessageStorage(self.db)
--        self.db.execute("""
--        DELETE FROM messages WHERE id = ?
--        """, id)
--
      @dbus.service.method('com.Gwibber.Test', out_signature='i')
      def SignalTestOn(self):
          self.connection_monitor.ConnectionOnline()
 === modified file 'gwibber/gwibber/tests/test_account.py'
 --- gwibber/gwibber/tests/test_account.py	2012-09-13 17:01:59 +0000
 +++ gwibber/gwibber/tests/test_account.py	2012-09-19 17:22:19 +0000
@@ -171,11 +171,6 @@
      """Test the AccountManager API."""
      def setUp(self):
--        self.stream = mock.Mock()
--        self.stream.List.return_value = json.dumps(
--            [dict(account='faker/than fake',
--                  id='foo'),
--            ])
          self.account_service = mock.Mock()
      def test_account_manager(self):
@@ -183,14 +178,14 @@
          # internal mapping.
          def refresh():
              pass
--        AccountManager(refresh, self.stream)
++        AccountManager(refresh)
      def test_account_manager_add_new_account(self):
          # Explicitly adding a new account puts the account's global_id into
          # the account manager's mapping.
          def refresh():
              pass
--        manager = AccountManager(refresh, self.stream)
++        manager = AccountManager(refresh)
          manager.add_new_account(self.account_service)
          self.assertIn('fake global id', manager._accounts)
@@ -201,7 +196,7 @@
              nonlocal refreshed
              refreshed = True
          accounts_manager.get_account().list_services.return_value = []
--        manager = AccountManager(refresh, self.stream)
++        manager = AccountManager(refresh)
          manager._on_enabled_event(accounts_manager, 'fake global id')
          self.assertTrue(refreshed)
@@ -213,7 +208,7 @@
          def refresh():
              nonlocal refreshed
              refreshed = True
--        manager = AccountManager(refresh, self.stream)
++        manager = AccountManager(refresh)
          self.assertNotIn('fake global id', manager._accounts)
          manager._on_account_deleted(accounts_manager, 'fake global id')
          self.assertNotIn('fake global id', manager._accounts)
@@ -226,11 +221,9 @@
          def refresh():
              nonlocal refreshed
              refreshed = True
--        manager = AccountManager(refresh, self.stream)
++        manager = AccountManager(refresh)
          manager.add_new_account(self.account_service)
          # Now manager._accounts has a 'fake global id' key.
          manager._on_account_deleted(accounts_manager, 'fake global id')
--        self.stream.List.assert_called_once_with()
--        self.stream.Delete.assert_called_once_with('foo')
          self.assertNotIn('fake global id', manager._accounts)
          self.assertTrue(refreshed)
 === modified file 'gwibber/gwibber/tests/test_dbus.py'
 --- gwibber/gwibber/tests/test_dbus.py	2012-09-12 22:15:43 +0000
 +++ gwibber/gwibber/tests/test_dbus.py	2012-09-19 17:22:19 +0000
@@ -67,24 +67,6 @@
      def setUp(self):
          self.session_bus = dbus.SessionBus()
--        # We have to make sure to remove any rows that the individual tests
--        # may have added, otherwise there are order dependencies.
--        self.stream_ids = []
--        self.search_ids = []
--
--    def tearDown(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Streams',
--            '/com/gwibber/Streams')
--        iface = dbus.Interface(obj, 'com.Gwibber.Streams')
--        for id in self.stream_ids:
--            iface.Delete(id)
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        for id in self.search_ids:
--            iface.Delete(id)
      def test_shorten(self):
          obj = self.session_bus.get_object(
@@ -94,125 +76,6 @@
          short_url = iface.Shorten('http://www.python.org')
          self.assertIsInstance(short_url, dbus.String)
--    def test_streams_list_empty(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Streams',
--            '/com/gwibber/Streams')
--        iface = dbus.Interface(obj, 'com.Gwibber.Streams')
--        self.assertEqual(iface.List(), '[]')
--
--    def test_streams_create(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Streams',
--            '/com/gwibber/Streams')
--        iface = dbus.Interface(obj, 'com.Gwibber.Streams')
--        name = 'bob'
--        account = 'twitter'
--        operation = 'tweet'
--        data = 'payload'
--        search = json.dumps(dict(name=name, account=account,
--                                 operation=operation, data=data))
--        id = iface.Create(search)
--        self.stream_ids.append(id)
--        self.assertEqual(iface.List(), '[payload]')
--
--    def test_streams_create_two(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Streams',
--            '/com/gwibber/Streams')
--        iface = dbus.Interface(obj, 'com.Gwibber.Streams')
--        name = 'bob'
--        account = 'twitter'
--        operation = 'tweet'
--        search = json.dumps(dict(name=name, account=account,
--                                 operation=operation, data='one'))
--        id = iface.Create(search)
--        self.stream_ids.append(id)
--        search = json.dumps(dict(name=name, account=account,
--                                 operation=operation, data='two'))
--        id = iface.Create(search)
--        self.stream_ids.append(id)
--        self.assertEqual(iface.List(), '[one, two]')
--
--    def test_streams_delete(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Streams',
--            '/com/gwibber/Streams')
--        iface = dbus.Interface(obj, 'com.Gwibber.Streams')
--        name = 'bob'
--        account = 'twitter'
--        operation = 'tweet'
--        data = 'payload'
--        search = json.dumps(dict(name=name, account=account,
--                                 operation=operation, data=data))
--        id = iface.Create(search)
--        self.assertEqual(iface.List(), '[payload]')
--        iface.Delete(id)
--        self.assertEqual(iface.List(), '[]')
--
--    def test_streams_get(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Streams',
--            '/com/gwibber/Streams')
--        iface = dbus.Interface(obj, 'com.Gwibber.Streams')
--        name = 'bob'
--        account = 'twitter'
--        operation = 'tweet'
--        data = 'getme'
--        search = json.dumps(dict(name=name, account=account,
--                                 operation=operation, data=data))
--        id = iface.Create(search)
--        self.assertEqual(iface.Get(id), 'getme')
--        self.stream_ids.append(id)
--
--    # XXX Need tests for the Updated(), Deleted(), and Created() signals in
--    # com.Gwibber.Streams.
--
--    def test_messages_get(self):
--        # First, insert some data into the messages table.  We do this through
--        # the dbus testing interface so that we don't have to worry about
--        # SQLite lock contention.
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Test',
--            '/com/gwibber/Test')
--        test_iface = dbus.Interface(obj, 'com.Gwibber.Test')
--        stored_ids = {}
--        stored_ids[test_iface.InsertDataIntoMessages('one')] = 'one'
--        stored_ids[test_iface.InsertDataIntoMessages('two')] = 'two'
--        # Now get the data back through the non-testing API.
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Messages',
--            '/com/gwibber/Messages')
--        iface = dbus.Interface(obj, 'com.Gwibber.Messages')
--        for key, value in stored_ids.items():
--            self.assertEqual(iface.Get(key), value)
--        # Clean up.
--        for id in stored_ids:
--            test_iface.DeleteDataFromMessages(id)
--
--    def test_messages_get_missing(self):
--        # First, insert some data into the messages table.  We do this through
--        # the dbus testing interface so that we don't have to worry about
--        # SQLite lock contention.
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Test',
--            '/com/gwibber/Test')
--        test_iface = dbus.Interface(obj, 'com.Gwibber.Test')
--        stored_ids = []
--        stored_ids.append(test_iface.InsertDataIntoMessages('one'))
--        stored_ids.append(test_iface.InsertDataIntoMessages('two'))
--        # Now get the data back through the non-testing API.
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Messages',
--            '/com/gwibber/Messages')
--        iface = dbus.Interface(obj, 'com.Gwibber.Messages')
--        self.assertEqual(iface.Get('9999'), '')
--        # Clean up.
--        for id in stored_ids:
--            test_iface.DeleteDataFromMessages(id)
--
--    # XXX Need test for the Message() signal in com.Gwibber.Messages.
--
      def test_connection_is_connected(self):
          # Test whether the network manager is connected or not.  This uses the
          # standard system service, and we can't guarantee its state will be
@@ -242,106 +105,6 @@
      # tests for various failure modes in ConnectionMonitor.__init__() and
      # .isConnected().
--    def test_searches_list_empty(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        self.assertEqual(iface.List(), '[]')
--
--    def test_searches_create(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        name = 'bob'
--        query = 'myquery'
--        data = 'payload'
--        search = json.dumps(dict(name=name, query=query, data=data))
--        id = iface.Create(search)
--        self.search_ids.append(id)
--        self.assertEqual(iface.List(), '[payload]')
--
--    def test_searches_create_two(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        name = 'bob'
--        query = 'myquery'
--        search = json.dumps(dict(name=name, query=query, data='one'))
--        id = iface.Create(search)
--        self.search_ids.append(id)
--        search = json.dumps(dict(name=name, query=query, data='two'))
--        id = iface.Create(search)
--        self.search_ids.append(id)
--        self.assertEqual(iface.List(), '[one, two]')
--
--    def test_searches_delete(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        name = 'bob'
--        query = 'myquery'
--        data = 'payload'
--        search = json.dumps(dict(name=name, query=query, data=data))
--        id = iface.Create(search)
--        self.assertEqual(iface.List(), '[payload]')
--        iface.Delete(id)
--        self.assertEqual(iface.List(), '[]')
--
--    def test_searches_delete_transients(self):
--        # Deleting search data also deletes rows from the messages table where
--        # the transients column matches the id in the storage table.  Call
--        # into the test service to set up the data for this.
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Test',
--            '/com/gwibber/Test')
--        test_iface = dbus.Interface(obj, 'com.Gwibber.Test')
--        stored_id_1 = test_iface.InsertSearchTransientData(
--            'anne', 'query1', 'one')
--        stored_id_2 = test_iface.InsertSearchTransientData(
--            'bart', 'query2', 'two')
--        # Now there should be two rows in both tables.
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        searches_iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        self.assertEqual(searches_iface.List(), '[one, two]')
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Messages',
--            '/com/gwibber/Messages')
--        messages_iface = dbus.Interface(obj, 'com.Gwibber.Messages')
--        self.assertEqual(messages_iface.Get(stored_id_1), 'one')
--        self.assertEqual(messages_iface.Get(stored_id_2), 'two')
--        # Now, delete just the first id.
--        searches_iface.Delete(stored_id_1)
--        # And verify that only 'two' exists in both tables.
--        self.assertEqual(searches_iface.List(), '[two]')
--        self.assertEqual(messages_iface.Get(stored_id_1), '')
--        self.assertEqual(messages_iface.Get(stored_id_2), 'two')
--        # Deleting the second one should clean everything back up.
--        searches_iface.Delete(stored_id_2)
--        self.assertEqual(searches_iface.List(), '[]')
--        self.assertEqual(messages_iface.Get(stored_id_1), '')
--        self.assertEqual(messages_iface.Get(stored_id_2), '')
--
--    def test_searches_get(self):
--        obj = self.session_bus.get_object(
--            'com.Gwibber.Searches',
--            '/com/gwibber/Searches')
--        iface = dbus.Interface(obj, 'com.Gwibber.Searches')
--        name = 'bob'
--        query = 'myquery'
--        data = 'getme'
--        search = json.dumps(dict(name=name, query=query, data=data))
--        id = iface.Create(search)
--        self.assertEqual(iface.Get(id), 'getme')
--        self.search_ids.append(id)
--
--    # XXX Need tests for the Updated(), Deleted(), and Created() signals.
--
      def test_get_features(self):
          obj = self.session_bus.get_object(
              'com.Gwibber.Service',
 === added file 'gwibber/gwibber/tests/test_model.py'
 --- gwibber/gwibber/tests/test_model.py	1970-01-01 00:00:00 +0000
 +++ gwibber/gwibber/tests/test_model.py	2012-09-19 17:22:19 +0000
@@ -0,0 +1,39 @@
++# Copyright (C) 2012 Canonical Ltd
++#
++# This program is free software: you can redistribute it and/or modify
++# it under the terms of the GNU General Public License version 2 as
++# published by the Free Software Foundation.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program.  If not, see <http://www.gnu.org/licenses/>.
++
++"""Test the Dee.SharedModel that we use for communicating with our frontend."""
++
++__all__ = [
++    'TestSharedModel',
++    ]
++
++
++import unittest
++
++from gi.repository import Dee
++from gwibber.utils.model import SharedModel
++
++
++class TestSharedModel(unittest.TestCase):
++    """Test our Dee.SharedModel instance."""
++
++    def test_basic_properties(self):
++        self.assertIsInstance(SharedModel, Dee.SharedModel)
++        self.assertEqual(SharedModel.get_n_columns(), 39)
++        self.assertEqual(SharedModel.get_n_rows(), 0)
++        self.assertEqual(SharedModel.get_schema(),
++                         ['aas', 's', 's', 's', 's', 'b', 's', 's', 's', 's',
++                          's', 's', 's', 's', 's', 's', 'd', 'b', 's', 's',
++                          's', 's', 's', 's', 's', 's', 's', 's', 's', 's',
++                          's', 's', 's', 's', 's', 's', 's', 's', 's'])
 === modified file 'gwibber/gwibber/tests/test_protocols.py'
 --- gwibber/gwibber/tests/test_protocols.py	2012-09-13 17:50:35 +0000
 +++ gwibber/gwibber/tests/test_protocols.py	2012-09-19 17:22:19 +0000
@@ -23,10 +23,24 @@
  import unittest
  import threading
++from gi.repository import Dee
++
  from gwibber.protocols.flickr import Flickr
  from gwibber.protocols.twitter import Twitter
  from gwibber.utils.base import Base
  from gwibber.utils.manager import ProtocolManager
++from gwibber.utils.model import SharedModel, COLUMN_TYPES
++
++
++try:
++    # Python 3.3
++    from unittest import mock
++except ImportError:
++    import mock
++
++
++TestModel = Dee.SharedModel.new('com.Gwibber.TestSharedModel')
++TestModel.set_schema_full(COLUMN_TYPES)
  class TestProtocolManager(unittest.TestCase):
@@ -73,6 +87,9 @@
  class TestProtocols(unittest.TestCase):
      """Test protocol implementations."""
++    def setUp(self):
++        TestModel.clear()
++
      def test_protocol_info(self):
          # Each protocol carries with it a number of protocol variables.  These
          # are initialized to None in the base class.
@@ -126,6 +143,84 @@
                  thread.join()
          self.assertEqual(my_protocol.result, 'one:two')
++    @mock.patch('gwibber.utils.base.SharedModel', TestModel)
++    def test_shared_model_successfully_mocked(self):
++        self.assertEqual(SharedModel.get_n_rows(), 0)
++        self.assertEqual(TestModel.get_n_rows(), 0)
++        base = Base(None)
++        base._publish('a', 'b', 'c', message='a',
++                      from_me=True, likes=1, liked=True)
++        base._publish('a', 'b', 'c', message='b',
++                      from_me=True, likes=1, liked=True)
++        base._publish('a', 'b', 'c', message='c',
++                      from_me=True, likes=1, liked=True)
++        self.assertEqual(SharedModel.get_n_rows(), 0)
++        self.assertEqual(TestModel.get_n_rows(), 3)
++
++    @mock.patch('gwibber.utils.base.SharedModel', TestModel)
++    def test_invalid_argument(self):
++        base = Base(None)
++        self.assertEqual(0, TestModel.get_n_rows())
++        self.assertRaises(TypeError, base._publish, invalid_argument='not good')
++
++    @mock.patch('gwibber.utils.base.SharedModel', TestModel)
++    @mock.patch('gwibber.utils.base._seen_messages', {})
++    def test_one_message(self):
++        base = Base(None)
++        self.assertEqual(0, TestModel.get_n_rows())
++        self.assertTrue(base._publish(
++            service='facebook', account_id='1/facebook',
++            message_id='1234', stream='messages', sender='fred',
++            sender_nick='freddy', from_me=True, timestamp='today',
++            message='hello, @jimmy', likes=10, liked=True))
++        self.assertEqual(1, TestModel.get_n_rows())
++
++    @mock.patch('gwibber.utils.base.SharedModel', TestModel)
++    @mock.patch('gwibber.utils.base._seen_messages', {})
++    def test_duplicate_messages_identified(self):
++        base = Base(None)
++        self.assertEqual(0, TestModel.get_n_rows())
++        self.assertTrue(base._publish(
++            service='facebook', account_id='1/facebook',
++            message_id='1234', stream='messages', sender='fred',
++            sender_nick='freddy', from_me=True, timestamp='today',
++            message='hello, @jimmy', likes=10, liked=True))
++
++        self.assertTrue(base._publish(
++            service='twitter', account_id='2/twitter',
++            message_id='5678', stream='messages', sender='fred',
++            sender_nick='freddy', from_me=True, timestamp='today',
++            message='hello jimmy', likes=10, liked=False))
++
++        self.assertEqual(1, TestModel.get_n_rows())
++        self.assertEqual(TestModel.get_row(TestModel.get_first_iter())[7],
++                         'hello, @jimmy')
++        self.assertEqual(TestModel.get_row(TestModel.get_first_iter())[0],
++            [['facebook', '1/facebook', '1234'],
++             ['twitter', '2/twitter', '5678']])
++
++    @mock.patch('gwibber.utils.base.SharedModel', TestModel)
++    @mock.patch('gwibber.utils.base._seen_messages', {})
++    def test_similar_messages_allowed(self):
++        base = Base(None)
++        self.assertEqual(0, TestModel.get_n_rows())
++
++        self.assertTrue(base._publish(
++            service='facebook', account_id='1/facebook',
++            message_id='1234', stream='messages', sender='fred',
++            sender_nick='freddy', from_me=True, timestamp='today',
++            message='hello, @jimmy', likes=10, liked=True))
++
++        self.assertEqual(1, TestModel.get_n_rows())
++
++        self.assertTrue(base._publish(
++            service='facebook', account_id='1/facebook',
++            message_id='34567', stream='messages', sender='tedtholomew',
++            sender_nick='teddy', from_me=False, timestamp='today',
++            message='hello, @jimmy', likes=10, liked=True))
++
++        self.assertEqual(2, TestModel.get_n_rows())
++
      def test_basic_login(self):
          # Try to log in twice.  The second login attempt returns False because
          # it's already logged in.
 === removed file 'gwibber/gwibber/tests/test_storage.py'
 --- gwibber/gwibber/tests/test_storage.py	2012-08-21 21:35:08 +0000
 +++ gwibber/gwibber/tests/test_storage.py	1970-01-01 00:00:00 +0000
@@ -1,688 +0,0 @@
--# Copyright (C) 2012 Canonical Ltd
--#
--# This program is free software: you can redistribute it and/or modify
--# it under the terms of the GNU General Public License version 2 as
--# published by the Free Software Foundation.
--#
--# This program is distributed in the hope that it will be useful,
--# but WITHOUT ANY WARRANTY; without even the implied warranty of
--# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
--# GNU General Public License for more details.
--#
--# You should have received a copy of the GNU General Public License
--# along with this program.  If not, see <http://www.gnu.org/licenses/>.
--
--"""Test stream storage."""
--
--__all__ = [
--    'TestMessageStorage',
--    'TestSearchStorage',
--    'TestStreamStorage',
--    ]
--
--
--import sqlite3
--import unittest
--
--from gwibber.storage.messages import MessageStorage
--from gwibber.storage.searches import SearchStorage
--from gwibber.storage.streams import StreamStorage
--
--import gwibber.storage.messages
--
--try:
--    # Python 3.3
--    from unittest import mock
--except ImportError:
--    import mock
--
--
--class TestStreamStorage(unittest.TestCase):
--    """Test stream storage."""
--
--    def setUp(self):
--        self.db = sqlite3.connect(':memory:')
--
--    def test_table_creation(self):
--        rows = self.db.execute('PRAGMA table_info(streams)').fetchall()
--        self.assertEqual(len(rows), 0)
--        StreamStorage(self.db)
--        rows = self.db.execute('PRAGMA table_info(streams)').fetchall()
--        # 5 rows because there are 5 columns in the table.
--        self.assertEqual(len(rows), 5)
--
--    def test_table_already_exists(self):
--        StreamStorage(self.db)
--        StreamStorage(self.db)
--        rows = self.db.execute('PRAGMA table_info(streams)').fetchall()
--        self.assertEqual(len(rows), 5)
--
--    def test_get_data(self):
--        storage = StreamStorage(self.db)
--        self.db.execute('INSERT INTO streams VALUES (?, ?, ?, ?, ?)',
--                        (1, 'myname', 'myaccount', 'operation', 'somedata'))
--        data = storage.get_data(1)
--        self.assertEqual(data, 'somedata')
--
--    def test_get_data_missing(self):
--        storage = StreamStorage(self.db)
--        self.db.execute('INSERT INTO streams VALUES (?, ?, ?, ?, ?)',
--                        (1, 'myname', 'myaccount', 'operation', 'somedata'))
--        data = storage.get_data(2)
--        self.assertEqual(data, '')
--
--    def test_list_all_data(self):
--        storage = StreamStorage(self.db)
--        with self.db:
--            for sample_data in [
--                (1, 'myname', 'myaccount', 'operation', 'somedata'),
--                (2, 'myname', 'myaccount', 'operation', 'moredata'),
--                (3, 'myname', 'myaccount', 'operation', 'anotherdata'),
--                ]:
--                self.db.execute(
--                    'INSERT INTO streams VALUES (?, ?, ?, ?, ?)',
--                    sample_data)
--        data = storage.list_all_data()
--        self.assertEqual(data, '[somedata, moredata, anotherdata]')
--
--    def test_add(self):
--        storage = StreamStorage(self.db)
--        storage.add(1, 'bob', 'ant', 'noop', 'data')
--        rows = self.db.execute('SELECT * FROM streams').fetchall()
--        self.assertEqual(len(rows), 1)
--        self.assertEqual(rows[0], ('1', 'bob', 'ant', 'noop', 'data'))
--
--    def test_remove(self):
--        storage = StreamStorage(self.db)
--        with self.db:
--            for sample_data in [
--                (1, 'myname', 'myaccount', 'operation', 'somedata'),
--                (2, 'myname', 'myaccount', 'operation', 'moredata'),
--                (3, 'myname', 'myaccount', 'operation', 'anotherdata'),
--                ]:
--                self.db.execute(
--                    'INSERT INTO streams VALUES (?, ?, ?, ?, ?)',
--                    sample_data)
--        storage.remove(2)
--        rows = self.db.execute('SELECT * FROM streams').fetchall()
--        self.assertEqual(len(rows), 2)
--        self.assertEqual([row[0] for row in rows], ['1', '3'])
--
--    def test_remove_missing(self):
--        storage = StreamStorage(self.db)
--        with self.db:
--            for sample_data in [
--                (1, 'myname', 'myaccount', 'operation', 'somedata'),
--                (2, 'myname', 'myaccount', 'operation', 'moredata'),
--                (3, 'myname', 'myaccount', 'operation', 'anotherdata'),
--                ]:
--                self.db.execute(
--                    'INSERT INTO streams VALUES (?, ?, ?, ?, ?)',
--                    sample_data)
--        storage.remove(5)
--        rows = self.db.execute('SELECT * FROM streams').fetchall()
--        self.assertEqual(len(rows), 3)
--        self.assertEqual([row[0] for row in rows], ['1', '2', '3'])
--
--
--class TestMessageStorage(unittest.TestCase):
--    """Test message storage."""
--
--    def setUp(self):
--        self.db = sqlite3.connect(':memory:')
--
--    def test_table_creation(self):
--        rows = self.db.execute('PRAGMA table_info(messages)').fetchall()
--        self.assertEqual(len(rows), 0)
--        MessageStorage(self.db)
--        rows = self.db.execute('PRAGMA table_info(messages)').fetchall()
--        # 14 rows because there are 14 columns in the table.
--        self.assertEqual(len(rows), 14)
--
--    def test_table_already_exists(self):
--        MessageStorage(self.db)
--        MessageStorage(self.db)
--        rows = self.db.execute('PRAGMA table_info(messages)').fetchall()
--        self.assertEqual(len(rows), 14)
--
--    def test_get_messages_sent_not_all(self):
--        # Test conditional arm 1.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '0', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', '', 100, '', 0, 0, '', '', 'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 1 and 2 only.  It will not return row
--        # 3 because its time is < 100.  It will not return row4 because its
--        # transient != '0'.  It will not return row5 because its from_me !=
--        # 1.  It will not return row6 because its account != 'ant'.
--        answer = storage.get_messages(
--            'sent', 'ant', 100, '0', '', 'time', 'ASC', 0)
--        self.assertEqual(answer, '[row1, row2]')
--
--    def test_get_messages_sent_not_all_order_desc(self):
--        # Test conditional arm 1 with descending order.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '0', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', '', 100, '', 0, 0, '', '', 'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 1 and 2 only.  It will not return row
--        # 3 because its time is < 100.  It will not return row4 because its
--        # transient != '0'.  It will not return row5 because its from_me !=
--        # 1.  It will not return row6 because its account != 'ant'.
--        answer = storage.get_messages(
--            'sent', 'ant', 100, '0', '', 'time', 'DESC', 0)
--        self.assertEqual(answer, '[row2, row1]')
--
--    def test_get_messages_sent_not_all_with_limit(self):
--        # Test conditional arm 1 with a limit.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '0', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', '', 100, '', 0, 0, '', '', 'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return row 1.  It will not return row 3 because
--        # its time is < 100.  It will not return row4 because its transient !=
--        # '0'.  It will not return row5 because its from_me != 1.  It will not
--        # return row6 because its account != 'ant'.
--        #
--        # In addition to all that, we'll get just row 1 due to LIMIT 1.
--        answer = storage.get_messages(
--            'sent', 'ant', 100, '0', '', 'time', 'ASC', 1)
--        self.assertEqual(answer, '[row1]')
--
--    def test_get_messages_sent_all(self):
--        # Test conditional arm 2.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '0', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', '', 100, '', 0, 0, '', '', 'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 1, 2, and 6.  It will not return row 3
--        # because its time is < 100.  It will not return row4 because its
--        # transient != '0'.  It will not return row5 because its from_me != 1.
--        answer = storage.get_messages(
--            'sent', 'all', 100, '0', '', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row1, row2, row6]')
--
--    def test_get_messages_not_transient_not_all_not_recipient(self):
--        # Test conditional arm 3, both legs of the OR clause.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '1', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', '', 100, '', 0, 0, '', '', 'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                (7, 7, 'twitter', '', '', '0', '', 100, '', 1, 0, '', 'bob',
--                                                                      'row7'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 4 and 7.  It will not return row3
--        # because its time is < 100.  It will not return any other rows
--        # because their transients are not '1' or their recipients are not
--        # 'bob'.
--        answer = storage.get_messages(
--            'stream', 'twitter', 100, '1', 'bob', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row4, row7]')
--
--    def test_get_messages_not_all_not_all(self):
--        # Test conditional arm 4.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '1', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', 'zee', 100, '', 0, 0, '', '',
--                                                                      'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return row 5.  That is the only row where the
--        # stream, account, time, and transient match the query.
--        answer = storage.get_messages(
--            'zee', 'ant', 100, '0', '0', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row5]')
--
--    def test_get_messages_all_not_all(self):
--        # Test conditional arm 5.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '1', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', 'zee', 100, '', 0, 0, '', '',
--                                                                      'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 1, 2 and 5.  All three rows match
--        # account ant, as well as time and transient.  Row 3 does not match
--        # the time, row 4 does not match the transient, row 6 does not match
--        # the account.
--        answer = storage.get_messages(
--            'all', 'ant', 100, '0', '0', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row1, row2, row5]')
--
--    def test_get_messages_not_all_all(self):
--        # Test conditional arm 6.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '1', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', 'zee', 100, '', 0, 0, '', '',
--                                                                      'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return row 5.  No other row has a matching stream.
--        answer = storage.get_messages(
--            'zee', 'all', 100, '0', '0', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row5]')
--
--    def test_get_messages_else(self):
--        # Test conditional arm 7.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '1', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', 'zee', 100, '', 0, 0, '', '',
--                                                                      'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                (7, 7, 'bee', '', '', '0', '', 99,  '', 1, 0, '', '', 'row7'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 1, 2, 5, and 6.  Those rows have a
--        # matching time and transient.  Rows 3 and 4 don't match transient,
--        # and row 7 doesn't match time.
--        answer = storage.get_messages(
--            'all', 'all', 100, '0', '0', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row1, row2, row5, row6]')
--
--    def test_get_messages_home(self):
--        # Test conditional arm 7, but by using the alias of stream == 'home'
--        # to mean stream == 'all' and account == 'all.  IOW, the account value
--        # is ignored.
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                (3, 3, 'ant', '', '', '1', '', 99,  '', 1, 0, '', '', 'row3'),
--                (4, 4, 'ant', '', '', '1', '', 100, '', 1, 0, '', '', 'row4'),
--                (5, 5, 'ant', '', '', '0', 'zee', 100, '', 0, 0, '', '',
--                                                                      'row5'),
--                (6, 6, 'bee', '', '', '0', '', 100, '', 1, 0, '', '', 'row6'),
--                (7, 7, 'bee', '', '', '0', '', 99,  '', 1, 0, '', '', 'row7'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        # This query should return rows 1, 2, 5, and 6.  Those rows have a
--        # matching time and transient.  Rows 3 and 4 don't match transient,
--        # and row 7 doesn't match time.
--        answer = storage.get_messages(
--            'home', 'cat', 100, '0', '0', 'data', 'ASC', 0)
--        self.assertEqual(answer, '[row1, row2, row5, row6]')
--
--    def test_get_data(self):
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        self.assertEqual(storage.get_data(2), 'row2')
--
--    def test_get_data_missing(self):
--        storage = MessageStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        self.assertEqual(storage.get_data(3), '')
--
--    def test_maintenance_no_accounts(self):
--        # Initialize the table.
--        MessageStorage(self.db)
--        # Calling maintenance() with no accounts should do nothing.
--        class FakeAccountManager:
--            def get_account(self, global_id):
--                return False
--        with self.db:
--            MessageStorage(self.db).maintenance(FakeAccountManager())
--        results = self.db.execute('SELECT account FROM messages').fetchall()
--        self.assertEqual(len(results), 0)
--
--    def test_maintenance_on_non_account(self):
--        # Initialize the table.
--        MessageStorage(self.db)
--        # Put some sample data into the messages table, but with an account
--        # that does not match the format expected by the maintenance()
--        # function, which it interprets as a missing account.  Every row gets
--        # deleted.
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant', '', '', '0', '', 100, '', 1, 0, '', '', 'row1'),
--                (2, 2, 'ant', '', '', '0', '', 101, '', 1, 0, '', '', 'row2'),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        class FakeAccountManager:
--            def get_account(self, global_id):
--                return False
--        with self.db:
--            MessageStorage(self.db).maintenance(FakeAccountManager())
--        results = self.db.execute('SELECT id FROM messages').fetchall()
--        self.assertEqual(len(results), 0)
--
--    def test_maintenance_on_missing_account(self):
--        # Initialize the table.
--        MessageStorage(self.db)
--        # Put some sample data into the messages table, but with accounts
--        # we'll treat as both existing, and missing.
--        # deleted.
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant/foo', '', '', '0', '', 100, '', 1, 0, '', '', ''),
--                (2, 2, 'ant/foo', '', '', '0', '', 101, '', 1, 0, '', '', ''),
--                (3, 3, 'bee/foo', '', '', '0', '', 100, '', 1, 0, '', '', ''),
--                (4, 4, 'bee/foo', '', '', '0', '', 101, '', 1, 0, '', '', ''),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        class FakeAccountManager:
--            def get_account(self, global_id):
--                return global_id == 'bee'
--        with self.db:
--            MessageStorage(self.db).maintenance(FakeAccountManager())
--        results = self.db.execute('SELECT id FROM messages').fetchall()
--        self.assertEqual(sorted(row[0] for row in results), ['3', '4'])
--
--    @mock.patch.dict(gwibber.storage.messages.__dict__, dict(MAX_COUNT=3))
--    def test_maintenance(self):
--        # Initialize the table.
--        MessageStorage(self.db)
--        # A bunch of messages in various accounts are in the table.  We're
--        # maxed out at three entries per account.  The 'ant' account has two
--        # entries, the 'bee' account has three, the 'cat' account has four,
--        # and the 'dog' account has five.  After maintenance() is run, no
--        # account has more than 3 entries, and all of them are the latest
--        # ones added.
--        with self.db:
--            for sample_data in [
--                # id, mid, account, service, operation, transient,
--                # stream, time, text, from_me, to_me, sender, recipient, data
--                (1, 1, 'ant/foo', '', 'receive', '0', 'messages',
--                                                    100, '', 1, 0, '', '', ''),
--                (2, 2, 'ant/foo', '', 'receive', '0', 'messages',
--                                                    101, '', 1, 0, '', '', ''),
--                (3, 3, 'bee/foo', '', 'receive', '0', 'messages',
--                                                    100, '', 1, 0, '', '', ''),
--                (4, 4, 'bee/foo', '', 'receive', '0', 'messages',
--                                                    101, '', 1, 0, '', '', ''),
--                (5, 5, 'bee/foo', '', 'receive', '0', 'messages',
--                                                    102, '', 1, 0, '', '', ''),
--                (6, 6, 'cat/foo', '', 'receive', '0', 'messages',
--                                                    100, '', 1, 0, '', '', ''),
--                (7, 7, 'cat/foo', '', 'receive', '0', 'messages',
--                                                    101, '', 1, 0, '', '', ''),
--                (8, 8, 'cat/foo', '', 'receive', '0', 'messages',
--                                                    102, '', 1, 0, '', '', ''),
--                (9, 9, 'cat/foo', '', 'receive', '0', 'messages',
--                                                    103, '', 1, 0, '', '', ''),
--                (10, 10, 'dog/foo', '', 'receive', '0', 'messages',
--                                                    100, '', 1, 0, '', '', ''),
--                (11, 11, 'dog/foo', '', 'receive', '0', 'messages',
--                                                    101, '', 1, 0, '', '', ''),
--                (12, 12, 'dog/foo', '', 'receive', '0', 'messages',
--                                                    102, '', 1, 0, '', '', ''),
--                (13, 13, 'dog/foo', '', 'receive', '0', 'messages',
--                                                    103, '', 1, 0, '', '', ''),
--                (14, 14, 'dog/foo', '', 'receive', '0', 'messages',
--                                                    104, '', 1, 0, '', '', ''),
--                ]:
--                self.db.execute("""
--                INSERT INTO messages VALUES
--                (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
--                """, sample_data)
--        class FakeAccountManager:
--            def get_account(self, global_id):
--                return True
--        with self.db:
--            MessageStorage(self.db).maintenance(FakeAccountManager())
--        results = self.db.execute(
--            'SELECT id, account FROM messages').fetchall()
--        ids_by_account = {}
--        for id, account in results:
--            global_id, slash, rest = account.partition('/')
--            ids_by_account.setdefault(global_id, []).append(int(id))
--        # There are fewer 'ant' messages than the maximum, so all are kept.
--        self.assertEqual(ids_by_account['ant'], [1, 2])
--        # There exactly the max number of 'bee' messages, so all are kept.
--        self.assertEqual(ids_by_account['bee'], [3, 4, 5])
--        # There is one too many 'cat' messages, so the earliest one (i.e. with
--        # id = 6, time = 100) is deleted.  The later ones are kept.
--        self.assertEqual(ids_by_account['cat'], [7, 8, 9])
--        # There are two too many 'dog' messages, so the earliest ones
--        # (i.e. with ids 10 and 11, times 100 and 101) are deleted.  The later
--        # ones are kept.
--        self.assertEqual(ids_by_account['dog'], [12, 13, 14])
--
--
--class TestSearchStorage(unittest.TestCase):
--    """Test the search storage."""
--
--    def setUp(self):
--        self.db = sqlite3.connect(':memory:')
--
--    def test_table_creation(self):
--        rows = self.db.execute('PRAGMA table_info(searches)').fetchall()
--        self.assertEqual(len(rows), 0)
--        SearchStorage(self.db)
--        rows = self.db.execute('PRAGMA table_info(searches)').fetchall()
--        # 4 rows because there are 4 columns in the table.
--        self.assertEqual(len(rows), 4)
--
--    def test_table_already_exists(self):
--        SearchStorage(self.db)
--        SearchStorage(self.db)
--        rows = self.db.execute('PRAGMA table_info(searches)').fetchall()
--        self.assertEqual(len(rows), 4)
--
--    def test_get_data(self):
--        storage = SearchStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, name, query, data
--                (1, 'anne', '', 'one'),
--                (2, 'bart', '', 'two'),
--                (3, 'cate', '', 'three'),
--                (4, 'dave', '', 'four'),
--                ]:
--                self.db.execute("""
--                INSERT INTO searches VALUES (?, ?, ?, ?)
--                """, sample_data)
--        data = storage.get_data(3)
--        self.assertEqual(data, 'three')
--
--    def test_get_data_missing(self):
--        storage = SearchStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, name, query, data
--                (1, 'anne', '', 'one'),
--                (2, 'bart', '', 'two'),
--                (3, 'cate', '', 'three'),
--                (4, 'dave', '', 'four'),
--                ]:
--                self.db.execute("""
--                INSERT INTO searches VALUES (?, ?, ?, ?)
--                """, sample_data)
--        data = storage.get_data(10)
--        self.assertEqual(data, '')
--
--    def test_list_all_data(self):
--        storage = SearchStorage(self.db)
--        with self.db:
--            for sample_data in [
--                # id, name, query, data
--                (1, 'anne', '', 'one'),
--                (2, 'bart', '', 'two'),
--                (3, 'cate', '', 'three'),
--                (4, 'dave', '', 'four'),
--                ]:
--                self.db.execute("""
--                INSERT INTO searches VALUES (?, ?, ?, ?)
--                """, sample_data)
--        data = storage.list_all_data()
--        self.assertEqual(data, '[one, two, three, four]')
--
--    def test_add(self):
--        storage = SearchStorage(self.db)
--        storage.add(1, 'bob', 'myquery', 'mydata')
--        rows = self.db.execute('SELECT * FROM searches').fetchall()
--        self.assertEqual(len(rows), 1)
--        id, name, query, data = rows[0]
--        self.assertEqual(id, '1')
--        self.assertEqual(name, 'bob')
--        self.assertEqual(query, 'myquery')
--        self.assertEqual(data, 'mydata')
--
--    def test_remove(self):
--        storage = SearchStorage(self.db)
--        with self.db:
--            for sample_data in [
--                (1, 'anne', 'myquery', 'one'),
--                (2, 'bart', 'myquery', 'two'),
--                (3, 'cate', 'myquery', 'three'),
--                ]:
--                self.db.execute(
--                    'INSERT INTO searches VALUES (?, ?, ?, ?)',
--                    sample_data)
--        storage.remove(2)
--        rows = self.db.execute('SELECT * FROM searches').fetchall()
--        self.assertEqual(len(rows), 2)
--        self.assertEqual([row[0] for row in rows], ['1', '3'])
--
--    def test_remove_missing(self):
--        storage = SearchStorage(self.db)
--        with self.db:
--            for sample_data in [
--                (1, 'anne', 'myquery', 'one'),
--                (2, 'bart', 'myquery', 'two'),
--                (3, 'cate', 'myquery', 'three'),
--                ]:
--                self.db.execute(
--                    'INSERT INTO searches VALUES (?, ?, ?, ?)',
--                    sample_data)
--        storage.remove(5)
--        rows = self.db.execute('SELECT * FROM searches').fetchall()
--        self.assertEqual(len(rows), 3)
--        self.assertEqual([row[0] for row in rows], ['1', '2', '3'])
 === modified file 'gwibber/gwibber/utils/account.py'
 --- gwibber/gwibber/utils/account.py	2012-09-13 17:50:35 +0000
 +++ gwibber/gwibber/utils/account.py	2012-09-19 17:22:19 +0000
@@ -29,7 +29,7 @@
  from gwibber.errors import UnsupportedProtocolError
  from gwibber.utils.manager import protocol_manager
--
++from gwibber.utils.model import SharedModel, COLUMN_NAMES
  log = logging.getLogger('gwibber.accounts')
@@ -37,9 +37,8 @@
  class AccountManager:
      """Manage the accounts that we know about."""
--    def __init__(self, refresh_callback, streams):
++    def __init__(self, refresh_callback):
          self._callback = refresh_callback
--        self._streams = streams
          self._accounts = {}
          # Ask libaccounts for a manager of the microblogging services.
          # Connect callbacks to the manager so that we can react when accounts
@@ -71,9 +70,11 @@
          except KeyError:
              log.debug('Attempting to delete unknown account: {}', account_id)
              return
--        for stream in json.loads(self._streams.List()):
--            if stream['account'] == account['id']:
--                self._streams.Delete(stream['id'])
++
++        for row in SharedModel:
++            if account_id in row[COLUMN_NAMES.index('message_ids')]:
++                SharedModel.remove(row)
++
          self._callback()
      def add_new_account(self, account_service):
 === modified file 'gwibber/gwibber/utils/base.py'
 --- gwibber/gwibber/utils/base.py	2012-09-13 17:50:35 +0000
 +++ gwibber/gwibber/utils/base.py	2012-09-19 17:22:19 +0000
@@ -20,8 +20,34 @@
+     ]
--from threading import Thread
--
++import string
++
++from threading import Thread, Lock
++
++from gwibber.utils.model import SharedModel, COLUMN_INDICES
++from gwibber.utils.model import SENDER, MESSAGE, IDS
++
++
++IGNORED = string.punctuation + ' '
++
++def strip(message):
++    """Return a string suitable for fuzzy comparisons.
++
++    This is used to identify highly similar but not quite identical duplicate
++    messages, so that we don't display those duplicates to the user.
++    """
++    message = message.replace('http://', '')
++    message = message.replace('https://', '')
++    message = message.replace('gwibber:/', '')
++    return ''.join(char for char in message if char not in IGNORED)
++
++
++# This dict maps messages stripped by the above function to the
++# DeeModelIters that represent their rows in the Dee.SharedModel.
++# It's used for convenience in finding duplicates.
++_seen_messages = {}
++
++_publish_lock = Lock()
  class Base:
      class info:
@@ -53,6 +79,44 @@
          thread.daemon = True
          thread.start()
++    def _publish(self, service, account_id, message_id, **kwargs):
++        """Publish fresh data into the stream, without duplicating anything.
++
++        Returns True if the message was appended, or if the message was
++        already present, returns False if unable to append new messagee.
++        """
++        args = [''] * len(COLUMN_INDICES)
++        # See gwibber/utils/model.py for an explanation of this unique column.
++        args[IDS] = [[service, account_id, message_id]]
++
++        # The basic idea is that you call _publish() with keyword
++        # arguments only, making the calling code much more explicit,
++        # and then this converts it from an unordered dict into the
++        # ordered list required by SharedModel.append().
++        bad_kwargs = []
++        for column_name, value in kwargs.items():
++            index = COLUMN_INDICES.get(column_name)
++            if index is None:
++                bad_kwargs.append(column_name)
++            else:
++                args[index] = value
++        if len(bad_kwargs) > 0:
++            raise TypeError('Unexpected keyword arguments: {}'.format(
++                    ', '.join(bad_kwargs)))
++
++        with _publish_lock:
++            # Don't let duplicate messages into the model, but do
++            # record the unique message ids of each duplicate message.
++            stripped = strip(args[SENDER] + args[MESSAGE])
++            row_iter = _seen_messages.get(stripped)
++            if row_iter is None:
++                _seen_messages[stripped] = SharedModel.append(*args)
++            else:
++                row = SharedModel.get_row(row_iter)
++                row[IDS] = row[IDS] + args[IDS]
++
++            return stripped in _seen_messages
++
      def _login(self):
          """Prevent redundant login attempts."""
          # The first time the user logs in, we expect old_token to be None.
 === added file 'gwibber/gwibber/utils/model.py'
 --- gwibber/gwibber/utils/model.py	1970-01-01 00:00:00 +0000
 +++ gwibber/gwibber/utils/model.py	2012-09-19 17:22:19 +0000
@@ -0,0 +1,109 @@
++# Copyright (C) 2012 Canonical Ltd
++#
++# This program is free software: you can redistribute it and/or modify
++# it under the terms of the GNU General Public License version 2 as
++# published by the Free Software Foundation.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program.  If not, see <http://www.gnu.org/licenses/>.
++
++"""The Dee.SharedModel interface layer.
++
++Dee.SharedModel is comparable to a Gtk.ListStore, except that it
++shares its state between processes using DBus. When gwibber-service
++downloads new messages from a website, it inserts those messages into
++this SharedModel instance, which then triggers a callback in the Vala
++frontend, which knows to display the new messages there.
++"""
++
++
++__all__ = [
++    'SharedModel',
++    'COLUMN_NAMES',
++    'COLUMN_TYPES',
++    'COLUMN_INDICES',
++    'SENDER',
++    'MESSAGE',
++    'IDS',
++    ]
++
++
++from gi.repository import Dee
++
++
++# Most of this schema is very straightforward, but the 'message_ids'
++# column needs a bit of explanation:
++#
++# It is a two-dimensional array (ie, an array of arrays). Each inner
++# array contains three elements: the name of the protocol, the
++# account_id (like '6/flickr' or '3/facebook'), followed by the
++# message_id for that particular service.
++#
++# Then, there will be one of these triples present for every service
++# on which the message exists. So for example, if the user posts the
++# same message to both facebook and twitter, that message will appear
++# as a single row in this schema, and the 'service' column will look
++# something like this:
++#
++# [
++#     ['facebook', '2/facebook', '12345'],
++#     ['twitter', '3/twitter', '987654'],
++# ]
++SCHEMA = (
++    ('message_ids',    'aas'),
++    ('stream',         's'),
++    ('transient',      's'),
++    ('sender',         's'),
++    ('sender_nick',    's'),
++    ('from_me',        'b'),
++    ('timestamp',      's'),
++    ('message',        's'),
++    ('html',           's'),
++    ('icon_uri',       's'),
++    ('url',            's'),
++    ('source',         's'),
++    ('timestring',     's'),
++    ('reply_nick',     's'),
++    ('reply_name',     's'),
++    ('reply_url',      's'),
++    ('likes',          'd'),
++    ('liked',          'b'),
++    ('retweet_nick',   's'),
++    ('retweet_name',   's'),
++    ('retweet_id',     's'),
++    ('link_picture',   's'),
++    ('link_name',      's'),
++    ('link_url',       's'),
++    ('link_desc',      's'),
++    ('link_caption',   's'),
++    ('link_icon',      's'),
++    ('img_url',        's'),
++    ('img_src',        's'),
++    ('img_thumb',      's'),
++    ('img_name',       's'),
++    ('video_pic',      's'),
++    ('video_src',      's'),
++    ('video_url',      's'),
++    ('video_name',     's'),
++    ('comments',       's'),
++    ('recipient',      's'),
++    ('recipient_nick', 's'),
++    ('recipient_icon', 's'),
++    )
++
++
++COLUMN_NAMES, COLUMN_TYPES = zip(*SCHEMA)
++
++COLUMN_INDICES = {name: i for i, name in enumerate(COLUMN_NAMES)}
++SENDER = COLUMN_INDICES['sender']
++MESSAGE = COLUMN_INDICES['message']
++IDS = COLUMN_INDICES['message_ids']
++
++
++SharedModel = Dee.SharedModel.new('com.Gwibber.Streams')
++SharedModel.set_schema_full(COLUMN_TYPES)

Gwibber

Merge lp:~robru/gwibber/dee into lp:~barry/gwibber/py3

Commit message

Description of the change

Preview Diff

Subscribers