Review Request: SpeedDial for Konqueror

[David Faure]

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



/trunk/KDE/kdebase/apps/konqueror/speeddial/part.cpp
<http://reviewboard.kde.org/r/2767/#comment3470>

    remove Q_UNUSED line, the arg is used below (which is good) ;)



/trunk/KDE/kdebase/apps/konqueror/speeddial/part.cpp
<http://reviewboard.kde.org/r/2767/#comment3471>

    I recommend QXmlStreamReader rather than QDomDocument for new code, especially if a sequential parsing is enough. It's much faster and less memory hungry.



/trunk/KDE/kdebase/apps/konqueror/speeddial/part.cpp
<http://reviewboard.kde.org/r/2767/#comment3472>

    missing &



/trunk/KDE/kdebase/apps/konqueror/speeddial/renderthread.h
<http://reviewboard.kde.org/r/2767/#comment3477>

    missing & after QString



/trunk/KDE/kdebase/apps/konqueror/speeddial/renderthread.cpp
<http://reviewboard.kde.org/r/2767/#comment3479>

    Ouch. You can't use KHTMLPart (which creates a widget) nor KIO, from a thread.
    
    This implementation needs to be de-threadified: use signals and slots instead.



/trunk/KDE/kdebase/apps/konqueror/speeddial/renderthread.cpp
<http://reviewboard.kde.org/r/2767/#comment3480>

    where is this pixmap deleted? Seems leaked to me.
    Use QPixmap by value rather than by pointer, it's much simpler.



/trunk/KDE/kdebase/apps/konqueror/src/konqmainwindow.cpp
<http://reviewboard.kde.org/r/2767/#comment3511>

    maybe using about:speeddial as internal url, instead of speeddial:, would simplify this, reducing the amount of changes needed? E.g. this one wouldn't be needed.
    
    And more generally, this would benefit from kio_about (which provides an actual implementation of the about: protocol, but I forgot in which case this was actually necessary).


- David


On 2010-01-30 12:28:16, Thomas Fischer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2767/
> -----------------------------------------------------------
> 
> (Updated 2010-01-30 12:28:16)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch adds a "speed dial" part to Konqueror. Within Konqueror it can be chosen as an alternative to a blank or an about start page. The speeddial part is accessible via the speeddial: url. This mimics the functionality of Opera' SpeedDial or the ShowCase add-on for Firefox.
> The correspondig bug report is https://bugs.kde.org/show_bug.cgi?id=149596
> 
> The code has still some minor issues such as UI blocking and missing access to Konqueror's history.
> Usability experts are invited to make comments on look&feel... ;-)
> 
> An older version of this code (without proper integration in Konqueror) has been presented here: http://www.unix-ag.uni-kl.de/~fischer/speeddial/index.html
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/apps/konqueror/CMakeLists.txt 1082172 
>   /trunk/KDE/kdebase/apps/konqueror/settings/konqhtml/generalopts.cpp 1082172 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/CMakeLists.txt PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/konq_speeddial.desktop PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/konqspeeddial.desktop PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/part.h PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/part.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/renderthread.h PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/renderthread.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/speeddial.xml PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/speeddialview.h PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/speeddialview.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/speeddialwidget.h PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/speeddial/speeddialwidget.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/apps/konqueror/src/konqfactory.cpp 1082172 
>   /trunk/KDE/kdebase/apps/konqueror/src/konqmainwindow.cpp 1082172 
>   /trunk/KDE/kdebase/apps/konqueror/src/konqmisc.cpp 1082172 
>   /trunk/KDE/kdebase/apps/konqueror/src/konqviewmanager.cpp 1082172 
> 
> Diff: http://reviewboard.kde.org/r/2767/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> SpeedDial in Konqueror
>   http://reviewboard.kde.org/r/2767/s/306/
> 
> 
> Thanks,
> 
> Thomas
> 
>