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 <[EMAIL PROTECTED]> 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 > <[EMAIL PROTECTED]> 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@lists.laptop.org > http://lists.laptop.org/listinfo/sugar > > _______________________________________________ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar