Review Request: SpeedDial for Konqueror

[David Faure]

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



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

    K*Action doesn't seem to be used in this file



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

    You're reimplementing QList::indexOf(), this would method could be done as a one-liner:
     return m_frames.indexOf(child, m_frames.count());



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

    Create the QMouseEvent on the stack, then there is no need for new+delete.



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

    QTextStream just for one readLine call is maybe overkill; QFile has readLine too, it just returns a bytearray so you have to decode it correctly (which was missing here as well).
    Possibly something like
    m_title = QString::fromUtf8(file.readLine());



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

    Note that this breaks if the title or the url contains a %.
    
    The usual solution is to use arg(a,b,c,d) instead of arg(a).arg(b).arg(c).arg(d), if possible. (You just need to make all the arguments QStrings).



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

    please use update() rather than repaint() if possible. repaint is too immediate, not compressed, and therefore possibly costly.



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

    Could it happen that the group is empty? In that case this loop should probably be a while() { ...} rather than a do+while.



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

    Ouch! processEvents is bad. Very dangerous. Unexpected re-entrancy, crashes, destruction of all life on earth, that kind of thing.
    
    What does it 'fix'? There are ways to do this more safely, for sure. If this is about posted events, a qApp->sendPostedEvents(the right widget, the right type of event) can help.



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

    Maybe this should use KUriFilter instead, if you want the same "short url" support as the konqueror location bar uses?


- David


On 2010-02-05 07:17:45, Thomas Fischer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2767/
> -----------------------------------------------------------
> 
> (Updated 2010-02-05 07:17:45)
> 
> 
> 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/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
> 
>