Little modifications on print.c

Dirk Hohndel dirk at hohndel.org
Mon Nov 5 09:26:09 PST 2012


Salvador Cuñat <salvador.cunat at gmail.com> writes:

> Good night,  I have made some little changes on print.c to fit my needs of
> a bigger hardcopy of the dives.
> Perhaps they can be of some utility for the project.
>
> - Add an option for a single dive per page  (not full A4 nor A5 but a size
> similar to some divelogs).
> - Changed the position of the profile to the lower of the print.
> - Add a thin frame for every dive.
>
> The patches apply sequentially over 2.1, first single dive, then the frame.
>
> Regards and thanks for all your efforts.

Hi Salva, thanks for your patch. Some comments:

> diff --git a/print.c b/print.c
> index e0fd284..5fd11d0 100644
> --- a/print.c
> +++ b/print.c
> @@ -68,17 +68,19 @@ static void show_dive_text(struct dive *dive, cairo_t *cr, double w,
>  
>  	utc_mkdate(dive->when, &tm);
>  	len = snprintf(buffer, sizeof(buffer),
> -		/*++GETTEXT 80 chars: lead text ("" or localized "Dive #%d - ") weekday, monthname, day, year, hour, min */

Please don't remove comments like this - they help the translators
understand what's happening around a specific message

>  		tm.tm_mday, tm.tm_year + 1900,
>  		tm.tm_hour, tm.tm_min);
> -
> -	set_font(layout, font, FONT_LARGE, PANGO_ALIGN_LEFT);
> -	pango_layout_set_text(layout, buffer, len);
> -	pango_layout_get_size(layout, &width, &height);
> +	if (print_options.type == ONEPERPAGE){                                                               /* - */
> +		set_font(layout, font, FONT_LARGE, PANGO_ALIGN_LEFT);
> +		} else {
> +			set_font(layout, font, FONT_NORMAL, PANGO_ALIGN_LEFT);
> +			}                                                                           /* - */
> +		pango_layout_set_text(layout, buffer, len);
> +		pango_layout_get_size(layout, &width, &height);

Have you looked at the existing coding style? Placement of braces is
very consistent in the existing code, please don't break that. Also,
what are these comments with just a '-' in them?

> @@ -92,9 +94,11 @@ static void show_dive_text(struct dive *dive, cairo_t *cr, double w,
>  
>  	depth = get_depth_units(dive->maxdepth.mm, &decimals, &unit);
>  	snprintf(buffer, sizeof(buffer),
> -		_("Max depth: %.*f %s\nDuration: %d min\n%s"),
> +		_("Max depth: %.*f %s\nDuration: %d min\nTª minima: %d\nS.A.C.: %d l/m\n%s"),

No, our text in the code is always English and then localized in translations.

>  		decimals, depth, unit,
>  		(dive->duration.seconds+59) / 60,
> +		(dive->watertemp.mkelvin / 1000) - 272,                                  /*Add water T ºC rounded up 1ºC*/
> +		(dive->sac / 1000) + 1,                                                  /*Add S.A.C rounded up 1 liter*/

No - we allow the user to configure the units they want to use - please
look at the get_temp_units function for this

> @@ -115,8 +119,11 @@ static void show_dive_text(struct dive *dive, cairo_t *cr, double w,
>  	maxheight -= height;
>  	pango_layout_set_height(layout, 1);
>  	pango_layout_set_width(layout, width);
> -
> -	set_font(layout, font, FONT_NORMAL, PANGO_ALIGN_LEFT);
> +	if (print_options.type == ONEPERPAGE){                                            /* Bigger fonts for larger print*/
> +		set_font(layout, font, FONT_LARGE, PANGO_ALIGN_LEFT);
> +		} else {
> +			set_font(layout, font, FONT_NORMAL, PANGO_ALIGN_LEFT);
> +			}                                                                 /* Normal fonts for normal print */

Again, placement of the curly braces - I'll stop commenting on that
below

> @@ -483,18 +521,31 @@ static GtkWidget *print_dialog(GtkPrintOperation *operation, gpointer user_data)
>  	box = gtk_hbox_new(FALSE, 2);
>  	gtk_container_add(GTK_CONTAINER(frame), box);
>  
> -	radio1 = gtk_radio_button_new_with_label (NULL, _("Pretty print"));
> +	radio1 = gtk_radio_button_new_with_label (NULL, _("6 dives per page"));
>  	gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(radio1),
>  		print_options.type == PRETTY);
> +
>  	radio2 = gtk_radio_button_new_with_label_from_widget (
> -		GTK_RADIO_BUTTON (radio1), _("Table print"));
> +		GTK_RADIO_BUTTON (radio1), _("1 dive per page"));
>  	gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(radio2),
> +		print_options.type == ONEPERPAGE);                                        /*Create the new button*/
> +
> +	radio3 = gtk_radio_button_new_with_label_from_widget (
> +		GTK_RADIO_BUTTON (radio1), _("Table print"));
> +	gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(radio3),
>  		print_options.type == TABLE);
> +
> +
> +

we use one empty line to help structure the code, but not two or three
of them - that wastes space and makes it harder to read the code.

Your second patch then goes on with some more white space issues - and
removes a few comments that the first patch added. I find that a
bit... odd.


Summary: I like what you are doing; I'm not sure I'm in love with the
changed layout for the six-up printing. But the coding style issues,
localization and user customization issues certainly stop me from
applying the change.

What I would be willing to look at again:

1) one patch that does nothing but add the ONEPERPAGE format (with
correct indentation, correct localization, correct use of units)
2) one patch that does the changes to the 6-up format

Thanks

/S


More information about the subsurface mailing list