desktop: option to change display of decimals of depth in dive table

[kruegerha]

The qthelper get_depth_string does now what is expected, depending on the given bool showdecimal (<20m exception removed). A preference (units.show_mdecimal_table) was introduced to change whether decimals are displayed for (metric) max depth values in the dive list. The setting can be change via UI at preferences/units.

This is a more general approach to #4431.

Describe the pull request:

• Bug fix
• Functional change
• New feature
• Code cleanup
• Build system change
• Documentation change
• Language translation

Pull request long description:

Changes made:

Related issues:

Additional information:

Release note:

Documentation change:

Mentions:

[github-actions]

Artifacts:
Subsurface-Windows-6.0.5330-patch.1.pull-request.decimal2
WARNING: Use at your own risk.

[github-actions]

Artifacts:
Subsurface-MacOS-6.0.5330-patch.1.pull-request.decimal2
WARNING: Use at your own risk.

[github-actions]

Artifacts:
Subsurface-Android-6.0.5330-patch.1.pull-request.decimal2
WARNING: Use at your own risk.

[mikeller]

This changes the behaviour in a lot of places (statistics, printing templates, the mobile version, ...), without giving the user a way to control the behaviour.
In all these places, a decimal will now be shown even for large depth numbers, which might result in truncated / overlapping display.

If this is intended then it will need a thorough testing.

[mikeller]

Our translations are managed in Transifex, and these files are auto-generated, so they should not be updated in pull requests

[kruegerha]

I don't think that this changes the behaviour in a lot of places.
The function get_depth_string takes the argument "showdecimal", so the calling code can chose whether to have decimals or not! This is not changed (only in a way that depths shallower than 20m will be covered by this choice too, which only shortens some of the delivered depth strings - we could keep the <20m exception, then nothing would change, although I think this behaviour is not what I would expect, when calling with showdecimals==false).

The major change by this patch is that the new prefs.unit.show_mdecimal_table is used in divetripmodel.cpp, where the main divelist table (desktop) is set up. As I understand the code, this will neither affect the statistics, printing templates nor the mobile version. However, for some of this places, the use of show_mdecimal_table could be tested later (printing templates?)

ok, for the translation stuff, I'll have to find out how and where to edit this.

[mikeller]

I don't think that this changes the behaviour in a lot of places. The function get_depth_string takes the argument "showdecimal", so the calling code can chose whether to have decimals or not! This is not changed (only in a way that depths shallower than 20m will be covered by this choice too, which only shortens some of the delivered depth strings - we could keep the <20m exception, then nothing would change, although I think this behaviour is not what I would expect, when calling with showdecimals==false).

showdecimalsin get_depth_string() seems to be hardcoded everywhere at the moment, or the default of trueused. With your proposed changes

• the false case will not change;
• in the truecase, a decimal will be shown for every depth, and not just for depths up to 20m, which is the current behaviour.

Looking at the code, a lot of the use of get_depth_string()seems to be using the true case:

➜  subsurface git:(master) ✗ grep -r get_depth_string
./mobile-widgets/qmlmanager.cpp:	if (get_depth_string(d->dcs[0].maxdepth.mm, true, true) != depth) {
./core/qthelper.cpp:QString get_depth_string(int mm, bool showunit, bool showdecimal)
./core/qthelper.cpp:QString get_depth_string(depth_t depth, bool showunit, bool showdecimal)
./core/qthelper.cpp:	return get_depth_string(depth.mm, showunit, showdecimal);
./core/qthelper.h:QString get_depth_string(depth_t depth, bool showunit = false, bool showdecimal = true);
./core/qthelper.h:QString get_depth_string(int mm, bool showunit = false, bool showdecimal = true);
./core/divelogexportlogic.cpp:			out << "\"AVG_DEPTH\":\"" << get_depth_string(s.avg_depth) << "\",";
./core/divelogexportlogic.cpp:			out << "\"MIN_DEPTH\":\"" << get_depth_string(s.min_depth) << "\",";
./core/divelogexportlogic.cpp:			out << "\"MAX_DEPTH\":\"" << get_depth_string(s.max_depth) << "\",";
./qt-models/diveimportedmodel.cpp:			return QVariant(get_depth_string(d.maxdepth.mm, true, false));
./qt-models/cylindermodel.cpp:			return get_depth_string(cyl->depth, true);
./qt-models/cylindermodel.cpp:				return get_depth_string(d->gas_mod(cyl->gasmix, modpO2, m_or_ft(1, 1)), true);
./qt-models/cylindermodel.cpp:				return get_depth_string(d->gas_mnd(cyl->gasmix, prefs.bestmixend, m_or_ft(1, 1)), true);
./qt-models/yearlystatisticsmodel.cpp:		return get_depth_string(stats_interval.avg_depth);
./qt-models/yearlystatisticsmodel.cpp:			return get_depth_string(stats_interval.combined_max_depth.mm / stats_interval.selection_size);
./qt-models/yearlystatisticsmodel.cpp:		return get_depth_string(stats_interval.min_depth);
./qt-models/yearlystatisticsmodel.cpp:		return get_depth_string(stats_interval.max_depth);
./qt-models/yearlystatisticsmodel.cpp:				QString label = QString(tr("%1 - %2")).arg(get_depth_string(i * (STATS_DEPTH_BUCKET * 1000), true, false),
./qt-models/yearlystatisticsmodel.cpp:						get_depth_string((i + 1) * (STATS_DEPTH_BUCKET * 1000), true, false));
./qt-models/divetripmodel.cpp:	case MobileListModel::DepthRole: return get_depth_string(d->dcs[0].maxdepth.mm, true, true);
./qt-models/divetripmodel.cpp:	case MobileListModel::DepthDurationRole: return QStringLiteral("%1 / %2").arg(get_depth_string(d->dcs[0].maxdepth.mm, true, true),
./qt-models/divetripmodel.cpp:			return get_depth_string(d->maxdepth, prefs.units.show_units_table);
./desktop-widgets/templatelayout.cpp:			return get_depth_string(object->avg_depth);
./desktop-widgets/templatelayout.cpp:			return get_depth_string(object->min_depth);
./desktop-widgets/templatelayout.cpp:			return get_depth_string(object->max_depth);
./desktop-widgets/templatelayout.cpp:			return get_depth_string(d->dcs[0].maxdepth.mm, true, true);
./desktop-widgets/templatelayout.cpp:			return get_depth_string(d->dcs[0].meandepth.mm, true, true);
./desktop-widgets/tab-widgets/TabDiveStatistics.cpp:		ui->depthLimits->setMaximum(get_depth_string(stats_selection.max_depth, true));
./desktop-widgets/tab-widgets/TabDiveStatistics.cpp:		ui->depthLimits->setMinimum(get_depth_string(stats_selection.min_depth, true));
./desktop-widgets/tab-widgets/TabDiveStatistics.cpp:		ui->depthLimits->setAverage(get_depth_string(stats_selection.combined_max_depth.mm / stats_selection.selection_size, true));
./desktop-widgets/tab-widgets/TabDiveStatistics.cpp:		ui->depthLimits->setAverage(get_depth_string(stats_selection.max_depth, true));
./desktop-widgets/tab-widgets/TabDiveInformation.cpp:	ui->maximumDepthText->setText(get_depth_string(currentDive->maxdepth, true));
./desktop-widgets/tab-widgets/TabDiveInformation.cpp:	ui->averageDepthText->setText(get_depth_string(currentDive->meandepth, true));
./desktop-widgets/tab-widgets/TabDiveNotes.cpp:		ui.depth->setText(get_depth_string(currentDive->maxdepth, true));
./desktop-widgets/tab-widgets/TabDiveNotes.cpp:	ui.depth->setText(get_depth_string(currentDive->maxdepth, true));
./profile-widget/diveprofileitem.cpp:	item->set(get_depth_string(entry.depth, true), color);
./profile-widget/diveprofileitem.cpp:	text->set(get_depth_string(lrint(lastMeanDepth), true), getColor(TEMP_TEXT));

[kruegerha]

see below... that's about what I found by grepping through the code...
my approach would be to change all these hardcoded true's to use the show_mdecimal_table property.
if then changing the behaviour in a way, that "false" would actually return decimals for depth <20m, the overall result should be exactly the same. The "true" case would then return decimals for all depths (which would need testing for the optical results).
I'll continue on that, as soon as I'm able to compile the mobile version... to do the required testing.

I consider this as a lesson in subsurface and its development tools, otherwise this would be a lot of wasted time for a couple of centimetres ;-/

[mikeller]

Ha, that's what I get for looking into emails before going to bed, and then responding first thing in the morning. 😆

my approach would be to change all these hardcoded true's to use the show_mdecimal_table property. if then changing the behaviour in a way, that "false" would actually return decimals for depth <20m, the overall result should be exactly the same.

Yes, that sounds good.
One thought about a potential alternative way of doing this: We could your bool preference to an int, and use it as 'Round to nearest meter for depths greater than' - this will give people flexibility instead of being '20m or nothing'.
And one more thought - do we want to grey out this preference if users have selected 'feet' as the depth unit?

The "true" case would then return decimals for all depths (which would need testing for the optical results). I'll continue on that, as soon as I'm able to compile the mobile version... to do the required testing.

Have a look at https://github.com/subsurface/subsurface/blob/master/packaging/android/README.md#in-a-container - the setup to build the android version is normally quite easy, as all that is needed is docker, and the build itself happens inside a container that comes with all prerequisites installed.

I consider this as a lesson in subsurface and its development tools, otherwise this would be a lot of wasted time for a couple of centimetres ;-/

Another way to look at this is that in volunteered open source development we get the liberty of obsessing over a couple of centimetres. 😉

[github-actions]

Artifacts:
Subsurface-Windows-6.0.5330-patch.2.pull-request.decimal2
WARNING: Use at your own risk.

[github-actions]

Artifacts:
Subsurface-MacOS-6.0.5330-patch.2.pull-request.decimal2
WARNING: Use at your own risk.

[github-actions]

Artifacts:
Subsurface-Android-6.0.5330-patch.2.pull-request.decimal2
WARNING: Use at your own risk.

[kruegerha]

ohh, silly me overlooked some calls to get_depth_string with the showdecimal=true argument.
Indeed, all the "output" should be using the _show_mdecimal_table option then. And of course that needs testing.
With using default prefs.unit.show_mdecimal_table == false, things shouldn't change much.

There is one 'internal' use of get_depth_string in a string comparison (in QMLManager::CheckDepth). I don't overlook, what the consequences will be, if decimals (<20m) change here... I'll further investigate and test...

[kruegerha]

The latest iteration uses explicit calls (regarding showdecimal) to get_depth_string everywhere.
User choice is applied to all on-screen occurrences of max depth of single dives. Statistics/averages, details, export, etc uses decimals.

The look of the dive list and profile is more tidy than before with a mix of decimals / no decimals.
No decimals should be the new default. Whoever is interested in decimeter-level depth information, is most like into this for every depth and can simply switch on the feature on the preferences/units tab.

For now, on mobile there are no decimals in the dive list/profile, once I found out how to mess with the mobile ui, I'll add that too.

Open for discussion and comments on this feature.

[github-actions]

Artifacts:
Subsurface-Windows-6.0.5330-patch.3.pull-request.decimal2
WARNING: Use at your own risk.

[github-actions]

Artifacts:
Subsurface-MacOS-6.0.5330-patch.3.pull-request.decimal2
WARNING: Use at your own risk.

[github-actions]

Artifacts:
Subsurface-Android-6.0.5330-patch.3.pull-request.decimal2
WARNING: Use at your own risk.

[mikeller]

The latest iteration uses explicit calls (regarding showdecimal) to get_depth_string everywhere. User choice is applied to all on-screen occurrences of max depth of single dives. Statistics/averages, details, export, etc uses decimals.

I am a bit wary of the potential of hitting Chesterton's fence with this one - there might be a reason why the existing behaviour was implemented, and users might rely on it. @dirkhh do you have any insights into this?

[dirkhh]

existing behavior was implemented because it reflected the general user preference.
One of our early design principles (and this one has been abandoned, drowned, doused in gasoline, set on fire, and subsequently stomped to ash) was that fewer options and user configurations was better.
In essence, every user controlled setting means something else that someone making changes has to test for.
None of you tests any of your changes with more than 5% of the possible configurations. And every new config option makes this worse and worse and worse.

In other words - the reason for this behavior was sanity and the sound reasoning that whatever we choose would be hated and loved by some subset of the users... and this one seemed to create a decent compromise.

[mikeller]

@kruegerha:

existing behavior was implemented because it reflected the general user preference.

That's what I suspected when I brought up Chesterton's fence.
So I think completely removing what was determined to be the users' preference isn't a great approach. What I see as a less disruptive solution will be to make the dept of the cut-off for the decimal configurable instead of going for a constant 20m, but keeping the default at 20m.