freedive mode

[Dirk Hohndel]

On Sun, Oct 04, 2015 at 05:00:02AM -0700, Giorgio Marzano wrote:
> Hi, all
> 
> could someone please explain me the workflow to propose, discuss and 
> implement a change?

I'd strongly suggest doing that on the developer mailing list, not in the
user forum.

> Let's say I have a change to propose: to improve readability I would like 
> the trips with duration < 1 day to be displayed in the form "Month Day Year 
> (dives)" instead of  "Month Year (dives)" 

I can see how that would make sense.

> Since this is simple, I also developed a signed off patch to do that 
> (according to http://subsurface-divelog.org/documentation/contributing/ ). 

Well, you didn't read all of the CodingStyle document it seems - we indent
with tabs (assumed to be 8 characters wide), not with 4 spaces. Some more
comments below

> What should I do in order to get the proposal discussed and the patch 
> reviewed and (eventually) merged? Should we discuss the proposal here 
> before I submit the patch? Should I send the patch here? Should I do a 
> "pull request"? How?

I tought the contribution document above explained that. Maybe it's not
clear enough?

You propose your idea and explain why (if it isn't obvious) in an email to
the mailing list. If you have a patch that you think is ready to be
included then make sure you have [PATCH] in the subject of the actual
email (not just in the attached patch as it was in your case). If you are
looking for feedback I recomment [RFC] or seomthing like that in the
subject line.

You'll get feedback and at some point I'll either say "nope, this doesn't
work for us", or I'll accept your patch.

> From 689f18187a9386945919c2d889ded933f4cc21d1 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"
> 
> This improves readability in the case that many trip refer to the same month.
> 
> Signed-off-by: Giorgio Marzano <marzano.giorgio at gmail.com>

That part looks good

> diff --git a/helpers.h b/helpers.h
> index 6c5c31c..06d68a4 100644
> --- a/helpers.h
> +++ b/helpers.h
> @@ -33,7 +33,7 @@ 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);
> +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..6104a1e 100644
> --- a/qt-models/divetripmodel.cpp
> +++ b/qt-models/divetripmodel.cpp
> @@ -43,6 +43,9 @@ static QVariant dive_table_alignment(int column)
>  QVariant TripItem::data(int column, int role) const
>  {
>  	QVariant ret;
> +	struct tm tmt, tmd;
> +	utc_mkdate(trip->when, &tmt);
> +    bool oneDayTrip=true;

Here we see the first whitespace / indentation issues

> @@ -59,14 +62,20 @@ QVariant TripItem::data(int column, int role) const
>  			while (d) {
>  				if (!d->hidden_by_filter)
>  					countShown++;
> -				d = d->next;
> +				utc_mkdate(d->when, &tmd);
> +			   if ((tmd.tm_mday != tmt.tm_mday) ||
> +					   (tmd.tm_mon != tmt.tm_mon) ||
> +					   (tmd.tm_year != tmt.tm_year)) {
> +					   oneDayTrip=false;
> +			   }
> +			   d = d->next;

More whitespace damage. Also it might be nicer to have a helper function
is_same_day(trip->when, d->when)
in that function you could have a static tmt and trip_when so you only
recreate the struct tm for the trip if trip->when != trip_when. And then
return a bool.
And in the data function you can then say
	oneDayTrip &= is_same_day(trip->when, d->when);
(notice that there is ALWAYS a space on either side of an arithmetic or
logical operator)

> diff --git a/qthelper.cpp b/qthelper.cpp
> index 31ec43d..d4a1e21 100644
> --- a/qthelper.cpp
> +++ b/qthelper.cpp
> @@ -1028,19 +1028,26 @@ 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)
> -{
> +QString get_trip_date_string(timestamp_t when, int nr, bool getday) {

Nope, the opening '{' of a function is at the beginning of the next line


So overall this looks like a good start and I think this is going to be a
reasonable change. I'd postpone this until after the 4.5 release which
gives you more time to get familiar with the coding style and the
appropriate settings for any IDE you might be using.

Makes sense?

Thanks

/D