[Marble-devel] Review Request 122686: Adds a right click menu to the statusbar that allows the user to quickly change the Angle display unit

[Calin Cruceru]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122686/#review76472
-----------------------------------------------------------


The code looks good, congratulations for your first patch! :-)

I opened a few issues, most of them concerning the coding style. Make sure you read and respect [Qt's coding style](http://qt-project.org/wiki/Qt_Coding_Style). This is the coding style Marble follows. There are only a few differences, the most important one (I mentioned it below as well) is that we usually leave a space before and after parameters/arguments in function definitions/calls.


src/apps/marble-qt/QtMainWindow.h
<https://git.reviewboard.kde.org/r/122686/#comment52671>

    Spaces after parantheses and the first and last parameters. Also, we could find a more meaningful name for this slot. Note that Qt's style is to use *ed terminated method names for signals. Since this is a slot, we could name it
    
    void updateStatusBarContextMenu( QPoint pos );



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52672>

    No need for a space before the open parenthesis in any function call (including constructors). This should be:
    
    **new QAction( tr( "Degree (DMS)" ), statusBar() );**
    
    Also, make sure you do the same below and everywhere this pattern applies.



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52673>

    A space after the open paranthesis and before the closed parenthesis.
    
    **m_dmsDegreeAction->setData( Marble::DMSDegree );**
    
    Also, make sure you do the same below and everywhere this pattern applies.



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52676>

    See Qt's coding style regarding the open brace.
    
    Also, characteristic to Marble's coding style, leave one space after the open parenthesis and before the closed one. Everywhere.
    
    This is not always respected in Marble's code base . Maybe we could fix that in the future. Consistency is very important. :-)



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52677>

    Those spaces before and after the -> operator doesn't look good at all, does it?
    
    **menu->addMenu( tr( "&Display Angle Unit" ) );**



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52678>

    Spaces. Should be:
    
    **switch ( m_configDialog->angleUnit() ) {**
    
    Note that the space after the switch keyword and before the open brace is Qt specific. The only thing Marble adds to this is the two spaces before and after the expression within the switch statement.



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52679>

    See Qt's coding style. Case statements should be alligned under the switch statement.



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52680>

    We usually do:
    
    connect( m_angleUnitMenu, SIGNAL(triggered(QAction*)), 
             m_configDialog, SLOT(statusBarChangesAngleUnit(QAction*)) );
    
    Note the spaces after the comma.



src/apps/marble-qt/QtMainWindow.cpp
<https://git.reviewboard.kde.org/r/122686/#comment52681>

    See above.



src/lib/marble/QtMarbleConfigDialog.h
<https://git.reviewboard.kde.org/r/122686/#comment52683>

    This slot makes the two classes, QtMarbleConfigDialog and QtMainWindow, to be too tightly coupled. Try changing this slot into
    
    **void updateAngleUnit( int newIndex );**
    
    and call it appropriately from QtMainWindow (add one more slot there if needed).


- Calin Cruceru


On Feb. 23, 2015, 2:13 p.m., Marius Stanciu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122686/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2015, 2:13 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Bugs: 344364
>     http://bugs.kde.org/show_bug.cgi?id=344364
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Menu contains degree(dms), degree(decimal) and utm formats, just like the settings dialog does.
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-qt/QtMainWindow.h 595ffd2 
>   src/apps/marble-qt/QtMainWindow.cpp c4280c6 
>   src/lib/marble/QtMarbleConfigDialog.h 1fbee43 
>   src/lib/marble/QtMarbleConfigDialog.cpp ed5b62c 
> 
> Diff: https://git.reviewboard.kde.org/r/122686/diff/
> 
> 
> Testing
> -------
> 
> it works on my machine.
> 
> 
> Thanks,
> 
> Marius Stanciu
> 
>

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