[Marble-devel] Review Request 124188: A patch for Bug 300184 - Eartquake Plugin: Need to use config dialog to see recent earth quakes

[Alexey Bugrov]

> On June 28, 2015, 8:40 a.m., Dennis Nienhüser wrote:
> > src/plugins/render/earthquake/EarthquakePlugin.cpp, line 167
> > <https://git.reviewboard.kde.org/r/124188/diff/1/?file=381446#file381446line167>
> >
> >     can we get rid of the if clause and just call
> >     `m_ui->m_timeRangeNPastDaysRadioButton->setChecked( m_timeRangeNPastDays );` ?
> 
> Alexey Bugrov wrote:
>     In this case,  m_timeRangeNPastDaysRadioButton is then always checked initially, irrespective of the m_timeRangeNPastDays in config. Anyway, is it all that important?
> 
> Dennis Nienhüser wrote:
>     It will start working when you put the radio buttons into the same buttongroup, and remove the manual setEnabled signal connection.

But they already are in the same buttongroup, mutually exclusive... The code suggested would just lead to them both being unchecked if m_timeRangeNPastDays is false. And signal connection is for the widgets to the right of each button to be grayed out when the given button is unchecked. I think it's better to gray them out, conceptually and aesthetically, no?


- Alexey


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


On June 27, 2015, 10:18 p.m., Alexey Bugrov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124188/
> -----------------------------------------------------------
> 
> (Updated June 27, 2015, 10:18 p.m.)
> 
> 
> Review request for Marble and Torsten Rahn.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> A patch for  Bug 300184 - Eartquake Plugin: Need to use config dialog to see recent earth quakes
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/earthquake/EarthquakeConfigWidget.ui 0cc1804 
>   src/plugins/render/earthquake/EarthquakePlugin.h 1cb88b7 
>   src/plugins/render/earthquake/EarthquakePlugin.cpp 47f041c 
> 
> Diff: https://git.reviewboard.kde.org/r/124188/diff/
> 
> 
> Testing
> -------
> 
> Done some testing, seems to work ok. Passes the unit tests too.
> 
> 
> Thanks,
> 
> Alexey Bugrov
> 
>

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