Review Request: Adds Firefox bookmark support to the bookmark runner

Fri Nov 6 19:14:21 CET 2009

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 64
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line64>
> >
> >     what's the use case for the configurability here?

If the runner detects the wrong profile and because of that uses the wrong database the user could change the database file manually. I think it additionally prevents from scanning the profile.ini every time.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 81
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line81>
> >
> >     would it make sense to do this check sooner? if there is no sqlite support, may as well avoid looking for the file altogether, right?

Yes, that's right, fixed. It's checked in the same method (reloadConfig()) but it's done first and the file check is only done if the driver is available.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 110
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line110>
> >
> >     probably shouldn't do this if the db wasn't open and/or the cache file doesn't exist?

Fixed. The remove is done if file.exists() is true.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, lines 251-253
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line251>
> >
> >     it looks like this only works in the firefox case? the check for all bookmarks should probably be moved to match() and passed in as a parameter to the get* (or match*? :) methods for consistency

This is made in Konqueror method, too. I only added this to the Firefox one because I saw it in the Konqueror method (line 198). I added a parameter bool allBookmarks for the methods.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 274
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line274>
> >
> >     is this used at all?

No, so I removed it. It was a rest of my old code.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 259
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line259>
> >
> >     since we have the power of SQL at our fingertips here, what about using the WHERE clause by adding, e.g. query("SELECT fk,title FROM moz_bookmarks WHERE type = 1 AND title LIKE  '" + term.replace("'", "\\'") + "'")

I did the point about the SQL query you mentioned below so I didn't do this one, because it didn't work with this.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 262
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line262>
> >
> >     this only matches the title; the konq version matches url as well. perhaps something for consistency?

Yeah, changed.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 276
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line276>
> >
> >     could this not be done with a join in the previous query? e.g.:
> >     
> >     SELECT moz_bookmarks.fk, moz_bookmarks.title, moz_places.url, moz_places.favicon_id FROM moz_bookmarks, moz_places WHERE moz_bookmarks.type = 1 and moz_bookmarks.fk = moz_places.id;

Done, too.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 151
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line151>
> >
> >     getKonquerorBookmarks, or matchKonquerorBookmarks?

I renamed the two methods to match*.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 130
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line130>
> >
> >     while we're in here, how about: getFavicon -> favicon

I renamed getFavicon to favicon

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, lines 120-127
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line120>
> >
> >     personally, i'd leave the context.addMatches call in each of the get* methods; less passing around and creation of lists

Done.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 73
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line73>
> >
> >     readEntry doesn't need to be templated for things like strings and ints. you can just do:
> >     
> >     readEntry("Path", QString());
> >

OK, thanks for the information. Fixed.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 66
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line66>
> >
> >     KSimpleConfig would be faster (skips merging, globals, etc)

KSimpleConfig doesn't exist anymore. I used KConfig("file", KConfig::SimpleConfig) as mentioned in ksimpleconfig.h

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 49
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line49>
> >
> >     extra white space should be removed.

This and all the other extra whit spaces are removed.

> On 2009-11-06 00:37:53, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp, line 48
> > <http://reviewboard.kde.org/r/2085/diff/1/?file=13824#file13824line48>
> >
> >     while we're in here, this sentence could probably be improved somewhat; perhaps "Lists all web browser bookmarks"?

Fixed.

- Jan Gerrit

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2085/#review2947
-----------------------------------------------------------

On 2009-11-05 19:20:27, Jan Gerrit Marker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2085/
> -----------------------------------------------------------
> 
> (Updated 2009-11-05 19:20:27)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch adds Firefox support to the bookmark runner. The database, where the bookmarks are saved in, is copied and then used for getting the bookmarks. The support was added a way that you can add other browsers easily (e.g. by adding a method "whichBrowser()", an enumeration "Browser" and own methods for each browser (Firefox and Konqueror at the moment)). The path to the original database file is stored in the runners configuration group. The browser to get the bookmarks from is taken from the command set in the applications systemsettings module.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/CMakeLists.txt 1045313 
>   trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.h 1045313 
>   trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1045313 
> 
> Diff: http://reviewboard.kde.org/r/2085/diff
> 
> 
> Testing
> -------
> 
> I tested it and it was fine with Firefox 3.5.4 and Konqueror from trunk.
> 
> 
> Thanks,
> 
> Jan Gerrit
> 
>