[PATCH] Trip header format in dive list

[Dirk Hohndel]

this should have gone to the developer mailing list...


Hi Giorgio,

I continue to review this a little more strictly than I might normally do,
simply to give you better ideas of what I prefer...

On Mon, Oct 05, 2015 at 11:04:51AM +0200, Giorgio Marzano wrote:
> From 29fde2898e04f839319e65d84092a6cb5e76a7f4 Mon Sep 17 00:00:00 2001
> From: Giorgio Marzano <marzano.giorgio at gmail.com>
> Date: Sun, 4 Oct 2015 13:48:10 +0200
> Subject: [PATCH] make trips shorter than 1 day displayed as "Month Day Year"

What's wrong with

[PATCH] include the day when showing single day trips in the divelist

it's active, it's more informative and it doesn't contain incorrect
information (as I sure home we don't always do middle endian...)

> This improves readability in the case that many trip refer to the same month.

many trips

> diff --git a/helpers.h b/helpers.h
> index 6c5c31c..760d962 100644
> --- a/helpers.h
> +++ b/helpers.h
> @@ -33,7 +33,8 @@ int parseTemperatureToMkelvin(const QString &text);
>  QString get_dive_duration_string(timestamp_t when, QString hourText, QString minutesText);
>  QString get_dive_date_string(timestamp_t when);
>  QString get_short_dive_date_string(timestamp_t when);
> -QString get_trip_date_string(timestamp_t when, int nr);
> +bool is_same_day (timestamp_t trip_when, timestamp_t dive_when);
> +QString get_trip_date_string(timestamp_t when, int nr, bool getday);
>  QString uiLanguage(QLocale *callerLoc);
>  QLocale getLocale();
>  QString getDateFormat();
> diff --git a/qt-models/divetripmodel.cpp b/qt-models/divetripmodel.cpp
> index fcba4a5..31d62ce 100644
> --- a/qt-models/divetripmodel.cpp
> +++ b/qt-models/divetripmodel.cpp
> @@ -43,6 +43,7 @@ static QVariant dive_table_alignment(int column)
>  QVariant TripItem::data(int column, int role) const
>  {
>  	QVariant ret;
> +   bool oneDayTrip=true;

And there we go again - four spaces instead of a tab

>  	if (role == DiveTripModel::TRIP_ROLE)
>  		return QVariant::fromValue<void *>(trip);
> @@ -59,14 +60,15 @@ QVariant TripItem::data(int column, int role) const
>  			while (d) {
>  				if (!d->hidden_by_filter)
>  					countShown++;
> -				d = d->next;
> +			    oneDayTrip &= is_same_day (trip->when,  d->when);
> +			   d = d->next;

Same here. Actually there's something even odder going on with the last line.

> diff --git a/qthelper.cpp b/qthelper.cpp
> index ed23948..4767be9 100644
> --- a/qthelper.cpp
> +++ b/qthelper.cpp
> @@ -1028,20 +1028,42 @@ const char *get_dive_date_c_string(timestamp_t when)
>  	return strdup(text.toUtf8().data());
>  }
>  
> -QString get_trip_date_string(timestamp_t when, int nr)
> +bool is_same_day(timestamp_t trip_when, timestamp_t dive_when)
> +{
> +	static timestamp_t twhen = (timestamp_t) 0;
> +	static struct tm tmt;
> +	struct tm tmd;
> +
> +	utc_mkdate(dive_when, &tmd);
> +
> +	if (twhen != trip_when) {
> +		twhen = trip_when;
> +		utc_mkdate(twhen, &tmt);
> +	}
> +
> +	return ((tmd.tm_mday == tmt.tm_mday) && (tmd.tm_mon == tmt.tm_mon)
> +	                && (tmd.tm_year == tmt.tm_year));

Up until the last line indentation was correct, the last one is all spaces
and off... the '&&' should be at the end of the previous line and the
"(tmd.tm_year..." should be aligned under the "(tmd.tm_mday..." above.
With as many tabs as possible and finally spaces

> +}
> +
> +QString get_trip_date_string(timestamp_t when, int nr, bool getday)
>  {
>  	struct tm tm;
>  	utc_mkdate(when, &tm);
> +	QDateTime localTime = QDateTime::fromTime_t(when);
> +	localTime.setTimeSpec(Qt::UTC);
> +	QString ret ;

No space before the ';'

> +
>  	if (nr != 1) {
> -		QString ret =  translate("gettextFromC", "%1 %2 (%3 dives)");
> -		return ret.arg(monthname(tm.tm_mon))
> -			.arg(tm.tm_year + 1900)
> -			.arg(nr);
> +		if (getday) {
> +			ret = localTime.date().toString(dateFormat).append(" (%1 dives)").arg(nr);
> +		} else {
> +			ret = localTime.date().toString("MMM yy").append(" (%1 dives)").arg(nr);
> +		}

This looks better

>  	} else {
> -		QString ret = translate("gettextFromC", "%1 %2 (1 dive)");
> -		return ret.arg(monthname(tm.tm_mon))
> -			.arg(tm.tm_year + 1900);
> +		ret = localTime.date().toString(dateFormat).append(" (1 dive)");

It took me a moment to realize that a trip with just one dive is by
definition always a single day trip :-)


Thanks for working on this - as I said, I'd like to leave this until after
4.5, but I think we are getting closer...

/D