I've started looking through the justification patch, and well, I
certainly haven't figured it all out yet. But I wanted to give some
feedback on stylistic issues in the patch.
Some general nits:
- Pango doesn't gint, but rather int, and so forth.
- Comments have a line of '*' down the left line.
/* Blah blah blah [...]
* blah
*/
The trailing */ goes on a separate line, except for single line
comments or when the comment applies to a single line of code.
(A comment preceding a function should have the */ on a separate
line even if it is a single line)
- Operators should be broken on the preceding line, not the
next line.
if (foo ||
bar)
I don't care for the GNU-recommended:
if (foo
|| bar)
- In general, there should be a line of white space between variable
declarations and code.
for (l = items; l; l = l->next)
{
PangoItem *item = l->data;
+
/* do something with item */
}
- Multiple declarations on the same line should only be used when
the elements are closely related.
GList *start_node, *end_node;
int x, y;
Are OK.
int last_tab_byte_offset = -1, i, j;
is not.
- GList variables should be named 'l', not 'elem'. (We used to use
tmp_list but that was clunky.) If you find you need two such
variables at once, the code probably needs division into separate
functions. If not, use a descriptive name like 'item_node'.
- I'd suggest always using double, and never float. I'd only
use floats in cases where space saving is a big issue.
Then turning to a few things I saw in the patch that I had difficulty
with stylistically:
===
/* This justifies the line of text, using the settings calculated in
finish_line(). */
static void
justify_line (PangoLayout *layout,
GSList *runs,
LineInfo *line_info,
JustificationInfo *justification_info,
PangoLayoutRun *line_start_run,
PangoLayoutRun *line_end_run,
gint *line_start_adjustment,
gint *line_end_adjustment)
===
We have 8 parameters here, some of with return values, some not, with no
documentation of what they do. I'd really like to keep functions to no
more than about 5 parameters. (Less is better.) If more are needed,
that almost certainly means that some sort of structure is needed.
If a functions parameters are non-obvious they should be documented;
that can be done informally; though I occasionally resort to gtk-doc
style comments even for static functions. (Internally exported functions
that are used across file boundaries always get doc comments)
Then we come to layout_with_specific_width. To quote the local variables
defined in it:
====
GArray *items_data, single_items_data;
ItemData *item_data, single_item_data;
LineInfo line_info = { 0 };
PangoItem *item;
const gchar *byte_pos = NULL, *line_end_byte_pos = NULL;
PangoGlyphUnit required_line_width, saved_line_width, natural_line_width;
PangoGlyphUnit total_distance = 0, break_distance = 0;
PangoGlyphUnit max_compression = 0, log_width, min_letter_spacing;
PangoGlyphUnit best_break_natural_line_width = 0;
PangoGlyphUnit total_compressible_width = 0, break_compressible_width = 0;
PangoLogAttr log_attr;
gboolean use_line_breaks = TRUE, use_char_breaks = TRUE;
gboolean trim_overflow = FALSE, overflowed = FALSE, need_full_copy = FALSE;
gint line_start_item_index = 0, item_index = 0;
gint char_offset, line_end_char_offset;
gint log_widths_char_num = 0, char_num = 0;
gint is_line_break = 0, is_char_break = 0, break_score;
gint last_tab_byte_offset = -1, i, j;
gint overflow_byte_offset = -1, overflow_char_offset = -1;
/* These are the total widths of glyphs, resizable space and number f
clusters up to the current position, the preferred break position, nd
the point we overflowed the line. (See the comments on TOTAL, O_BREAK
and OVERFLOW above.) */
PangoGlyphUnit total_width[NUM_SCRIPT_GROUPS][3];
PangoGlyphUnit resizable_width[NUM_SCRIPT_GROUPS][3];
gint num_clusters[NUM_SCRIPT_GROUPS][3];
/* This is the information we keep for the best break found so far. */
gint best_break_item_index = -1, best_break_score = G_MAXINT;
gboolean best_break_is_line_break = FALSE;
gint best_break_log_widths_char_num = -1;
====
Obviously this is doing something really hard and complicated. But we
need to figure out how to split up 421 line function with 44 local
variables.
The approach I've taken elsewhere - ellipsize.c, pango_itemize(), the
old line break code, was to use a structure to hold the state of the
operation - at that point, breaking stuff down into identifiable
functions becomes easier. And we also separate out the variables that
*aren't* state (i, j, say), from the state. The structure also provides
a convenient place to document the tricky part of the state.
justify_line() probably could also use some splitting up.
Finally, I would like to see this in a separate file. pango-layout.c is
pushing the bounds of maintainability already; I don't know how much
this actually adds, but it looks like between 1000 and 2000 lines.
I don't like asking you to do more work on this for purely stylistic
reasons, since you've already spent so much time just getting it
working, but I do think it is important to keep the code as readable
and maintainable as possible.
Regards,
Owen
Attachment:
signature.asc
Description: This is a digitally signed message part