* On 31 Aug 2016, Damien Riegel wrote:
> > /* Set CurrentMenu to MENU_MAIN before executing any folder
> > @@ -1270,6 +1273,8 @@
> > (option (OPTREADONLY) || op ==
> > OP_MAIN_CHANGE_FOLDER_READONLY) ?
> > MUTT_READONLY : 0, NULL)) != NULL)
> > {
> > + Labels = hash_create(131, 0);
>
> Why 131? Could you document/explain this value?
OK, will add. It's just a rough prime estimate of how many distinct
labels someone might have in a mailbox.
> > }
> > break;
> > }
> > + else if (flags & MUTT_LABEL && ch == OP_EDITOR_COMPLETE)
> > + {
> > + /* invoke the alias-menu to get more addresses */
> > + for (i = state->curpos; i && state->wbuf[i-1] != ',' &&
>
> Trailing whitespace, also this code use tabulations.
I'll remove this since you pointed it out, but FTR it's going to be
really hard in general to pick out trailing whitespace added by a patch
vs. trailing whitespace that was always there. Mutt's code is not
pretty in general.
> > @@ -162,6 +162,7 @@
> > WHERE const char *ReleaseDate;
> >
> > WHERE HASH *Groups;
> > +WHERE HASH *Labels;
>
> A bit off topic, what do people think of the `WHERE` macro? To me, it
> only makes tools like cscope confused and doesn't improve readability, I
> would really love to get rid of them at some point.
No objection. IIRC this is an old construct for supporting K&R and ANSI
in the same code. I'm not sure we need it anymore.
> > +static void label_ref_dec(char *label)
> > +{
> > + uintptr_t count;
>
> Why uintptr_t and not int or unsigned?
Because int and void* can't be inter-typecast on platforms where
pointers are 64-bit and ints are 32 (LP64). uintptr_t is actually the
correct standard type for an int that will be cast to pointer or vice
versa.
> > @@ -244,7 +277,7 @@
> > strncpy(buf, hdr->env->x_label, LONG_STRING);
> > }
> >
> > - if (mutt_get_field("Label: ", buf, sizeof(buf), 0 /* | MUTT_CLEAR */) !=
> > 0)
> > + if (mutt_get_field("Label: ", buf, sizeof(buf), MUTT_LABEL /* |
> > MUTT_CLEAR */) != 0)
>
> You should take this opportunity to remove the commented option here.
I declined to do that because I don't know why the comment is there.
I'm changing the line for relevance to this patch, but removing the
comment is not relevant to this patch. If it were on another line you'd
be asking me whether it belongs here. ;)
> > +
> > + if (Completed[0] == 0 && User_typed[0])
> > + return 0;
> > +
> > + /* Num_matched will _always_ be atleast 1 since the initial
> > + * user-typed string is always stored */
> > + if (numtabs == 1 && Num_matched == 2)
> > + snprintf(Completed, sizeof(Completed), "%s", Matches[0]);
> > + else if (numtabs > 1 && Num_matched > 2)
> > + /* cycle thru all the matches */
> > + snprintf(Completed, sizeof(Completed), "%s",
>
> Trailing whitespace.
Got it.
> > --- a/protos.h
> > +++ b/protos.h
> > @@ -187,6 +187,8 @@
> > void mutt_edit_headers (const char *, const char *, HEADER *, char *,
> > size_t);
> > int mutt_filter_unprintable (char **);
> > int mutt_label_message (HEADER *);
> > +void mutt_scan_labels (CONTEXT *);
> > +int mutt_label_complete (char *, size_t, int, int);
>
> It would make more sense to group them under a common "mutt_label"
> "namespace". So I would go with mutt_label_scan(_all eventually) and
> mutt_label_complete. It makes easier to find functions that work
> together.
Agreed. I'm not going to worry about it in this case because the
keywords patch changes all of this (and does better with consistency).
But this patch and some others are between here and there.
> > void mutt_curses_error (const char *, ...);
> > void mutt_curses_message (const char *, ...);
> > void mutt_encode_descriptions (BODY *, short);
> >
>
> Thanks,
Thank you!
--
David Champion • [email protected]
