Review Request: Adds Firefox bookmark support to the bookmark runner

[Aaron Seigo]

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



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.h
<http://reviewboard.kde.org/r/2085/#comment2336>

    extra white space should be removed.



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.h
<http://reviewboard.kde.org/r/2085/#comment2337>

    extra white space should be removed.



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.h
<http://reviewboard.kde.org/r/2085/#comment2338>

    extra white space should be removed.



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.h
<http://reviewboard.kde.org/r/2085/#comment2339>

    extra white space should be removed.



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.h
<http://reviewboard.kde.org/r/2085/#comment2340>

    extra white space should be removed.



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2342>

    while we're in here, this sentence could probably be improved somewhat; perhaps "Lists all web browser bookmarks"?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2341>

    extra white space should be removed.



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2343>

    what's the use case for the configurability here?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2344>

    KSimpleConfig would be faster (skips merging, globals, etc)



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2345>

    readEntry doesn't need to be templated for things like strings and ints. you can just do:
    
    readEntry("Path", QString());
    



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2346>

    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?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2347>

    probably shouldn't do this if the db wasn't open and/or the cache file doesn't exist?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2349>

    personally, i'd leave the context.addMatches call in each of the get* methods; less passing around and creation of lists



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2350>

    while we're in here, how about: getFavicon -> favicon



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2351>

    getKonquerorBookmarks, or matchKonquerorBookmarks?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2352>

    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



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2356>

    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("'", "\\'") + "'")



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2355>

    this only matches the title; the konq version matches url as well. perhaps something for consistency?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2353>

    is this used at all?



trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/2085/#comment2354>

    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;


- Aaron


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
> 
>