[sugar] [PATCH] Browse "autocompletion"

[Tomeu Vizoso]

Hi,

+        self._con = sqlite3.connect(db_path)
+        cur = self._con.cursor()

I'd avoid abbreviations if possible: self._connection and cursor.

+        cur.execute('insert into places values (?, ?, ?, ?, ?, ?)', \
+                    (place.uri, place.title, place.bookmark,
+                     place.gecko_flags, place.visits, place.last_visit))

This will break or have unintended effects if the schema changes.
Specifying the names of the fields will make it more robust:

'insert into places (uri, title, bookmark, gecko_flags, visits,
last_visit) values (?, ?, ?, ?, ?, ?)'

+        cur = self._con.cursor()
...
+        cur.close()

Should we use try...finally blocks so we don't leak open cursors?
Also, we could use the with statement from future.

+        place.uri, place.title, place.bookmark, place.gecko_flags, \
+            place.visits, place.last_visit = row

If we are going to use row as a tuple, we should specify the columns
we are querying in every select statement, so code is more resilient
to schema changes. If row could be used as a dict, then it wouldn't
matter.

+    COL_ADDRESS = 0
+    COL_TITLE = 1

Should be private?

+        self.handler_block(self._change_hid)
+        self.props.text = uri
+        self.handler_unblock(self._change_hid)

try...finally?

+        self.handler_block(self._change_hid)
+        self._address = address
+        self.handler_unblock(self._change_hid)

Do we really need to block the signal handler here? Same with self._title.

+        list_store = gtk.ListStore(str, str)
+
+        for place in places.get_store().search(self.props.text):
+            list_store.append([place.uri, place.title])

Might be interesting to implement a descendant of ListStore that
accesses directly the places db, so that the user can scroll down past
the maximum number of search results.

+    def __focus_in_event_cb(self, entry, event):
+        self.handler_block(self._change_hid)
+        self.props.text = self._address
+        self.handler_unblock(self._change_hid)

Could we avoid this code duplication?

+        path, col_dummy, x_dummy, y_dummy = \

I've been using col_ for dummies and pylint seems to like it.

Regards,

Tomeu

On Thu, Jun 12, 2008 at 11:22 PM, Marco Pesenti Gritti
<mpgritti at gmail.com> wrote:
> Added the bookmark field so that we don't have to change tables later.
>
> Marco
>
> On Thu, Jun 12, 2008 at 11:09 PM, Marco Pesenti Gritti
> <mpgritti at gmail.com> wrote:
>> Hello,
>>
>> the attached patch implements history and autocompletion. Terminology:
>> I'm calling a history/bookmark item place, similarly to firefox. I'm
>> also generally using search instead of autocompletion, because that's
>> what it actually does.
>>
>> Bookmarks are not hooked up, but it should be trivial to do so. I'd
>> actually appreciate if someone with some more understanding of the
>> bookmarks code in Browse could do that (Simon?). I think the idea is
>> that all the session bookmarks are added to the places store, even
>> those which are just shared by someone else but never visited. I think
>> we should add a bookmark:boolean to Place so that we can give those
>> higher priority in the queries.
>>
>> The entry patch is to sugar-toolkit, I'm moving adress/title handling
>> in web-activity.
>>
>> For splitted up commits see:
>> http://dev.laptop.org/git?p=users/marco/web-activity;a=summary
>>
>> Marco
>>
>
> _______________________________________________
> Sugar mailing list
> Sugar at lists.laptop.org
> http://lists.laptop.org/listinfo/sugar
>
>