[Marble-devel] Review Request 124540: Saves and restores panels state in settings

[Torsten Rahn]

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



src/apps/marble-ui/ControlView.cpp (line 306)
<https://git.reviewboard.kde.org/r/124540/#comment58762>

    I think a better name would be setAllPanelsVisible(bool).



src/apps/marble-ui/ControlView.cpp (line 318)
<https://git.reviewboard.kde.org/r/124540/#comment58764>

    see comment on the setter.



src/apps/marble-ui/ControlView.cpp (line 333)
<https://git.reviewboard.kde.org/r/124540/#comment58763>

    passing bools or ints in a list as a parameter is almost never a good idea. It's not obvious which panel is assigned which value since it's not really transparent what the order is.
    Also your method is named "State". What kind of state are you referring to? In this case judging by the name I'd expect the parameter to be a (Q)State. But that's probably not what you want to indicate. why not stay with visibility here?


- Torsten Rahn


On Juli 31, 2015, 2:38 vorm., Alexey Bugrov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124540/
> -----------------------------------------------------------
> 
> (Updated Juli 31, 2015, 2:38 vorm.)
> 
> 
> Review request for Marble, Dennis Nienhüser and Torsten Rahn.
> 
> 
> Bugs: 318401
>     http://bugs.kde.org/show_bug.cgi?id=318401
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This is for marble-qt, I'll add marble-kde patch if this one is accepted, since I cannot currently work on the kde app in master.
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-qt/QtMainWindow.cpp 3ae8471 
>   src/apps/marble-ui/ControlView.h caf99b8 
>   src/apps/marble-ui/ControlView.cpp 0fe1b21 
> 
> Diff: https://git.reviewboard.kde.org/r/124540/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexey Bugrov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150907/2fb41c74/attachment.html>