[Marble-devel] Review Request 110152: Fixed Marble Bug: Visibility of dock widgets should be togglable by single action

[Dennis Nienhüser]

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


Thanks for the patch. Great to see that you implemented it for both the Qt and the KDE application!

It would be great if you could reduce the code duplication a bit by some additional methods, or by moving some things to ControlView even (this one is shared by the Qt and KDE application).


src/KdeMainWindow.h
<http://git.reviewboard.kde.org/r/110152/#comment23680>

    const QList<QDockWidget*> &widgets



src/KdeMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23685>

    updateSideBarAction() sounds like a more descriptive name to me



src/KdeMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23682>

    Please use a more descriptive name (and no abbreviations). The local variable can be instantiated directly in the for loop as well.



src/KdeMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23683>

    local variables should not have a m_ prefix



src/KdeMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23684>

    this code does nothing at all: It just writes to local variables and changes no state.



src/KdeMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23681>

    this check is not needed, findChildren will not return 0 items



src/KdeMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23686>

    the method can be simplified to
    m_visibleDockWidgets = visibleDocks;



src/QtMainWindow.h
<http://git.reviewboard.kde.org/r/110152/#comment23688>

    This one shouldn't be needed



src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23689>

    a local variable is enough



src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23690>

    No abbreviations like sl please, use a more descriptive name like visibleDocks



src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23691>

    you can spare the second for loop here with a call to
    
    if ( sl.contains( allDocks.at(i)->objectName() ) ) {
    ...



src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23692>

    sl => visibleDocks



src/marble_part.h
<http://git.reviewboard.kde.org/r/110152/#comment23693>

    a local variable is enough for this one



src/marble_part.h
<http://git.reviewboard.kde.org/r/110152/#comment23694>

    can we spare this variable? if not, it needs a proper name (e.g. m_visibleDocks)



src/marble_part.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23695>

    can you rename it setShowSideBar, please?



src/marble_part.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23697>

    the parameter should be a const reference



src/marble_part.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23696>

    Please shorten to m_visibleDocksList = visibleDocks



src/marble_part.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23698>

    the parameter should be a const reference



src/marble_part.cpp
<http://git.reviewboard.kde.org/r/110152/#comment23699>

    can use contains() again to replace the for loop


- Dennis Nienhüser


On April 27, 2013, 2:41 p.m., Shiv Shankar Verma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110152/
> -----------------------------------------------------------
> 
> (Updated April 27, 2013, 2:41 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> Fixed bug in both marble and marble-qt
> 
> Adding toggling action to hide or show all the panels using a click Settings -> Show Panels
> also by shortcut F9
> 
> 
> This addresses bug 318401.
>     http://bugs.kde.org/show_bug.cgi?id=318401
> 
> 
> Diffs
> -----
> 
>   src/ControlView.h 083fbcf 
>   src/ControlView.cpp f66db9b 
>   src/KdeMainWindow.h 3e5b8ba 
>   src/KdeMainWindow.cpp 1e4b07a 
>   src/QtMainWindow.h b02cd36 
>   src/QtMainWindow.cpp cc69642 
>   src/marble.kcfg d3b5505 
>   src/marble_part.h 66aa484 
>   src/marble_part.cpp 5edc906 
>   src/marble_part.rc baa3618 
> 
> Diff: http://git.reviewboard.kde.org/r/110152/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Shiv Shankar Verma
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20130429/6514922d/attachment-0001.html>