On Tue, Aug 30, 2016 at 06:01:19PM -0700, d...@bikeshed.us wrote: > Adds capability to edit x-labels inside mutt, and to sort by label.
Hi David,
I have comments interleaved below. I haven't applied and actually
tested the patch (being quite confident that you and others have
probably been using this for quite a while).
> diff --git a/commands.c b/commands.c
> --- a/commands.c
> +++ b/commands.c
> @@ -533,9 +533,9 @@
> int method = Sort; /* save the current method in case of abort */
>
> switch (mutt_multi_choice (reverse ?
> - _("Rev-Sort
> (d)ate/(f)rm/(r)ecv/(s)ubj/t(o)/(t)hread/(u)nsort/si(z)e/s(c)ore/s(p)am?: ") :
> - _("Sort
> (d)ate/(f)rm/(r)ecv/(s)ubj/t(o)/(t)hread/(u)nsort/si(z)e/s(c)ore/s(p)am?: "),
> - _("dfrsotuzcp")))
> + _("Rev-Sort
> Date/Frm/Recv/Subj/tO/Thread/Unsort/siZe/sCore/sPam/Label?: ") :
> + _("Sort
> Date/Frm/Recv/Subj/tO/Thread/Unsort/siZe/sCore/sPam/Label?: "),
> + _("dfrsotuzcpl")))
Most of the other multi_choice menus use the parenthesized letters
paradigm. Unless there is a good reason, I'd rather not see that
changed as part of this patch. Was this because of 80-column width issues?
> diff --git a/copy.c b/copy.c
> --- a/copy.c
> +++ b/copy.c
> @@ -111,6 +111,10 @@
> ignore = 0;
> }
>
> + if (flags & CH_UPDATE_LABEL &&
> + mutt_strncasecmp ("X-Label:", buf, 8) == 0)
Make sure to use ascii_strncasecmp(), as in the other comparisons. (see
the BEWARE file).
Also, I would either add the "&& h->xlabel_changed" here too, or just
remove it from:
> + if (flags & CH_UPDATE_LABEL && h->xlabel_changed)
> + {
Since you are setting
> @@ -494,6 +507,9 @@
> _mutt_make_string (prefix, sizeof (prefix), NONULL (Prefix), Context,
> hdr, 0);
> }
>
> + if (hdr->xlabel_changed)
> + chflags |= CH_UPDATE_LABEL;
> +
the CH_UPDATE_LABEL conditionally based on the xlabel_changed being set.
> diff --git a/curs_main.c b/curs_main.c
> --- a/curs_main.c
> +++ b/curs_main.c
> @@ -2079,6 +2079,21 @@
> menu->redraw = REDRAW_FULL;
> break;
>
> + case OP_EDIT_LABEL:
> +
> + CHECK_MSGCOUNT;
> + CHECK_READONLY;
> + rc = mutt_label_message(tag ? NULL : CURHDR);
> + if (rc > 0) {
> + Context->changed = 1;
> + menu->redraw = REDRAW_FULL;
> + mutt_message ("%d label%s changed.", rc, rc == 1 ? "" : "s");
You should tag the above for localization. Since we haven't really
used the plural form extensions of gettext in mutt yet (e.g. ngettext),
I would suggest simply
mutt_message _("%d labels changed.", rc);
> diff --git a/headers.c b/headers.c
> +/*
> + * add an X-Label: field.
> + */
> +static int label_message(HEADER *hdr, char *new)
> +{
> + if (hdr == NULL)
> + return 0;
> + if (hdr->env->x_label == NULL && new == NULL)
> + return 0;
> + if (hdr->env->x_label != NULL && new != NULL &&
> + strcmp(hdr->env->x_label, new) == 0)
> + return 0;
> + if (hdr->env->x_label != NULL)
> + FREE(&hdr->env->x_label);
> + if (new == NULL)
> + hdr->env->x_label = NULL;
> + else
> + hdr->env->x_label = safe_strdup(new);
> + return hdr->changed = hdr->xlabel_changed = 1;
> +}
Using the lib functions, this can be simplfied a bit to just
static int label_message(HEADER *hdr, char *new)
{
if (hdr == NULL)
return 0;
if (mutt_strcmp (hdr->env->x_label, new) == 0)
return 0;
mutt_str_replace (&hdr->env->xlabel, new);
return hdr->changed = hdr->xlabel_changed = 1;
}
> diff --git a/pager.c b/pager.c
> --- a/pager.c
> +++ b/pager.c
> @@ -2807,6 +2807,18 @@
> redraw = REDRAW_FULL;
> break;
>
> + case OP_EDIT_LABEL:
> + CHECK_MODE(IsHeader (extra));
> + rc = mutt_label_message(extra->hdr);
> + if (rc > 0) {
> + Context->changed = 1;
> + redraw = REDRAW_FULL;
> + mutt_message ("%d label%s changed.", rc, rc == 1 ? "" :
> "s");
Same as above for curs_main.c
--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
