diff mbox series

[pacman-dev] util.c: table_print_line: properly align texts involving CJK

Message ID 20200912082558.363690-1-yan12125@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev] util.c: table_print_line: properly align texts involving CJK | expand

Commit Message

Chih-Hsuan Yen Sept. 12, 2020, 8:25 a.m. UTC 
Fixes FS#59229

Signed-off-by: Chih-Hsuan Yen <yan12125@gmail.com>
---
 src/pacman/util.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Andrew Gregory Sept. 12, 2020, 10:49 p.m. UTC | #1 
On 09/12/20 at 04:25pm, Chih-Hsuan Yen wrote:
> Fixes FS#59229
> 
> Signed-off-by: Chih-Hsuan Yen <yan12125@gmail.com>
> ---
>  src/pacman/util.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index e9187529..b45ca22d 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -516,22 +516,32 @@ static void table_print_line(const alpm_list_t *line, short col_padding,
>  			i++, curcell = alpm_list_next(curcell)) {
>  		const struct table_cell_t *cell = curcell->data;
>  		const char *str = (cell->label ? cell->label : "");
> -		int cell_width;
> +		int padding_width;
>  
>  		if(!has_data[i]) {
>  			continue;
>  		}
>  
> -		cell_width = (cell->mode & CELL_RIGHT_ALIGN ? (int)widths[i] : -(int)widths[i]);
> -
>  		if(need_padding) {
>  			printf("%*s", col_padding, "");
>  		}
>  
>  		if(cell->mode & CELL_TITLE) {
> -			printf("%s%*s%s", config->colstr.title, cell_width, str, config->colstr.nocolor);
> +			printf("%s", config->colstr.title);
> +		}
> +		/* Handle alignment manually here - for printf in C, "If the
> +		 * precision is specified, no more than that many bytes are
> +		 * written." [1]
> +		 * [1] Section 7.21.6, N2176, final draft for ISO/IEC 9899:2017 (C18)
> +		 * Fixes FS#59229. */

I appreciate a reference to a spec, but this is a bit verbose for
a source comment.  This information is better suited to the commit
message with a shorter comment in the source about printf width being
in bytes instead of characters.

I think this solution is more complex than necessary though.  It
should be sufficient to just adjust the above cell_width to be in
bytes rather than characters.  Something like:

        /* calculate cell width adjusting for multi-byte character strings */
		cell_width = (int)widths[i] + strlen(str) - string_length(str);
		cell_width = cell->mode & CELL_RIGHT_ALIGN ? cell_width : -cell_width;

> +		padding_width = (int)(widths[i] - string_length(str));
> +		if (cell->mode & CELL_RIGHT_ALIGN) {
> +			printf("%*s%s", padding_width, "", str);
>  		} else {
> -			printf("%*s", cell_width, str);
> +			printf("%s%*s", str, padding_width, "");
> +		}
> +		if(cell->mode & CELL_TITLE) {
> +			printf("%s", config->colstr.nocolor);
>  		}
>  		need_padding = 1;
>  	}
> -- 
> 2.28.0
Chih-Hsuan Yen Sept. 13, 2020, 8:43 a.m. UTC | #2 
Andrew Gregory <andrew.gregory.8@gmail.com> 於 2020年9月13日 週日 上午6:49寫道：
>
> On 09/12/20 at 04:25pm, Chih-Hsuan Yen wrote:
> > Fixes FS#59229
> >
> > Signed-off-by: Chih-Hsuan Yen <yan12125@gmail.com>
> > ---
> >  src/pacman/util.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/pacman/util.c b/src/pacman/util.c
> > index e9187529..b45ca22d 100644
> > --- a/src/pacman/util.c
> > +++ b/src/pacman/util.c
> > @@ -516,22 +516,32 @@ static void table_print_line(const alpm_list_t *line, short col_padding,
> >                       i++, curcell = alpm_list_next(curcell)) {
> >               const struct table_cell_t *cell = curcell->data;
> >               const char *str = (cell->label ? cell->label : "");
> > -             int cell_width;
> > +             int padding_width;
> >
> >               if(!has_data[i]) {
> >                       continue;
> >               }
> >
> > -             cell_width = (cell->mode & CELL_RIGHT_ALIGN ? (int)widths[i] : -(int)widths[i]);
> > -
> >               if(need_padding) {
> >                       printf("%*s", col_padding, "");
> >               }
> >
> >               if(cell->mode & CELL_TITLE) {
> > -                     printf("%s%*s%s", config->colstr.title, cell_width, str, config->colstr.nocolor);
> > +                     printf("%s", config->colstr.title);
> > +             }
> > +             /* Handle alignment manually here - for printf in C, "If the
> > +              * precision is specified, no more than that many bytes are
> > +              * written." [1]
> > +              * [1] Section 7.21.6, N2176, final draft for ISO/IEC 9899:2017 (C18)
> > +              * Fixes FS#59229. */
>
> I appreciate a reference to a spec, but this is a bit verbose for
> a source comment.  This information is better suited to the commit
> message with a shorter comment in the source about printf width being
> in bytes instead of characters.
>
> I think this solution is more complex than necessary though.  It
> should be sufficient to just adjust the above cell_width to be in
> bytes rather than characters.  Something like:
>
>         /* calculate cell width adjusting for multi-byte character strings */
>                 cell_width = (int)widths[i] + strlen(str) - string_length(str);
>                 cell_width = cell->mode & CELL_RIGHT_ALIGN ? cell_width : -cell_width;
>
> > +             padding_width = (int)(widths[i] - string_length(str));
> > +             if (cell->mode & CELL_RIGHT_ALIGN) {
> > +                     printf("%*s%s", padding_width, "", str);
> >               } else {
> > -                     printf("%*s", cell_width, str);
> > +                     printf("%s%*s", str, padding_width, "");
> > +             }
> > +             if(cell->mode & CELL_TITLE) {
> > +                     printf("%s", config->colstr.nocolor);
> >               }
> >               need_padding = 1;
> >       }
> > --
> > 2.28.0

Thanks a lot! That looks much better. I sent v2 based on your suggestion.

Best,

Chih-Hsuan Yen
diff mbox series

Patch

diff --git a/src/pacman/util.c b/src/pacman/util.c
index e9187529..b45ca22d 100644
--- a/src/pacman/util.c
+++ b/src/pacman/util.c
@@ -516,22 +516,32 @@  static void table_print_line(const alpm_list_t *line, short col_padding,
 			i++, curcell = alpm_list_next(curcell)) {
 		const struct table_cell_t *cell = curcell->data;
 		const char *str = (cell->label ? cell->label : "");
-		int cell_width;
+		int padding_width;
 
 		if(!has_data[i]) {
 			continue;
 		}
 
-		cell_width = (cell->mode & CELL_RIGHT_ALIGN ? (int)widths[i] : -(int)widths[i]);
-
 		if(need_padding) {
 			printf("%*s", col_padding, "");
 		}
 
 		if(cell->mode & CELL_TITLE) {
-			printf("%s%*s%s", config->colstr.title, cell_width, str, config->colstr.nocolor);
+			printf("%s", config->colstr.title);
+		}
+		/* Handle alignment manually here - for printf in C, "If the
+		 * precision is specified, no more than that many bytes are
+		 * written." [1]
+		 * [1] Section 7.21.6, N2176, final draft for ISO/IEC 9899:2017 (C18)
+		 * Fixes FS#59229. */
+		padding_width = (int)(widths[i] - string_length(str));
+		if (cell->mode & CELL_RIGHT_ALIGN) {
+			printf("%*s%s", padding_width, "", str);
 		} else {
-			printf("%*s", cell_width, str);
+			printf("%s%*s", str, padding_width, "");
+		}
+		if(cell->mode & CELL_TITLE) {
+			printf("%s", config->colstr.nocolor);
 		}
 		need_padding = 1;
 	}