[PATCHES] 4.5.5

[Robert Helling]

> On 23.03.2016, at 00:14, Dirk Hohndel <dirk at hohndel.org> wrote:
> 
> Thanks, Robert.
> 
> I have a couple of small concerns. Given that it's past midnight in
> Germany, I'll just fix them in your commit
> 
> /D
> 
>> diff --git a/desktop-widgets/diveplanner.cpp b/desktop-widgets/diveplanner.cpp
>> index e6fe612..32797f0 100644
>> --- a/desktop-widgets/diveplanner.cpp
>> +++ b/desktop-widgets/diveplanner.cpp
>> @@ -448,6 +447,18 @@ void PlannerSettingsWidget::settingsChanged()
>> 		ui.bottomSAC->setValue((double) prefs.bottomsac / 1000.0);
>> 		ui.decoStopSAC->setValue((double) prefs.decosac / 1000.0);
>> 	}
>> +	if(get_units()->pressure == units::BAR) {
>> +		ui.reserve_gas->setSuffix(tr("bar"));
>> +		ui.reserve_gas->setSingleStep(1);
>> +		ui.reserve_gas->setMaximum(5000);
> 
> step of 1 makes sense, a maximum of 5000 bar makes no sense. We used to
> allow up to 99bar here - I think we should keep that, or make it an even
> 100.
> 
>> +		ui.reserve_gas->setValue(prefs.reserve_gas / 1000);
>> +	} else {
>> +		ui.reserve_gas->setSuffix(tr("psi"));
>> +		ui.reserve_gas->setSingleStep(1);
> 
> Step of 1psi? I don't think so. Let's make that 10.
> 
>> +		ui.reserve_gas->setMaximum(5000);
> 
> 5000 psi reserve? How about 1500? Which is similar to the 100bar above.


Dirk,

you caught me here but your „solution“ actually makes it worse:

The problem is when switching from PSI to bar: You start, say with a value of 580psi (roughly 40bar). Then you change units, this makes the above code execute. Now you reduce the maximum to 100 which changes the value displayed from 580 to 100. This in turn sends a value changed signal. The receiving slots checks units and finds „bar“, and so interprets the 100 as 100bar  and sets the prefs.reserve_gas to 100000. And only then the above code is further executed but now thinks 100bar is what the user wants.

When I wrote this yesterday, I had (somewhat) reasonable values for the maximum first (after I encountered that there is a maximum, probably coming from the .ui file which is way to low for a psi value). The I ran into the issue I described above. But as you noted, this was around midnight and I was not able to solve this properly. So I just thought „what the heck“ and made the maximum values so big that they did not interfere anymore with reasonably sized reserves. This obviously was a hack of somebody who desperately wanted to go to bed and you caught me.

What I should have done instead would have been either

a) Move the setMaximim() call after the setValue() call in the metric branch which avoids the above issue (my thinking was not clear enough to realize this would have been the appropriate thing)

or

b) Get rid of the maximum value properly (not by making it ridiculously big but by also deleting it from the .ui file). In any case, what is it good for? Why do we want to prevent the user from doing stupid things? Because we could get crazy values when turning the mouse wheel? And what is an appropriate maximum? 100bar since no sane person would keep a reserve that big? Or 300bar since this is roughly the highest pressure we would ever encounter no matter if it makes sense as a reserve?

Please let me know your preference and I will send a new patch. Do you want one with —amend or a second one on top of your modification?

Best
Robert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20160323/5e30d8d1/attachment.sig>