Great feedback, Kevin - thanks.
* On 01 Jan 2016, Kevin J. McCarthy wrote:
> Patch 1 comments:
>
> 1. diff -r 9480a363a68a -r cd7be2bfff1c parse.c
> + if (url_check_scheme (beg) == U_MAILTO)
> + {
> + FREE (dst);
>
> You'll probably need to add a /* __FREE_CHECKED__ */ to this line to
> make check_sec.sh happy.Embarrassed to say I always forget check_sec.sh's peculiarities. Thanks. > 2. Is the List-Reply a common header too? I couldn't find it in the > RFC 2369 document, so I'm just wondering why it's being added and > preferred over List-Post? Huh. I would have sworn I actually read it in the document. Must have fabricated it from some other memory. You're right though; this doesn't belong. I'll remove it and fold the two patches. > Patch 2 comments: > > 1. It looks like you added the new Esc-L keybinding to main, pager, and > the attach menu. I only saw code handing it in curs_main.c. I see the > pager menu will bounce back out to the index menu, but I think it may be > worth adding code inside pager_menu() too, so they are still inside the > message if it was invoked from there. I don't know if it really makes Good point - > sense to have the action in the attach menu though (and it doesn't seem > to be working there in any case). and fair enough. Will amend as suggested. > 2. The ListHelp could use an entry for "Archives" OK. I only added the ones I thought would be used much, but there's room. No problem. > 3. In make_entry(), I think you need to add a _() translation marker > around ListActions[num].name so it's translated. (I noticed you flagged > the values with N_ above). I'm a complete amateur at i18n, but I think you must be right. > 4. The use of the offsetof() in the ListActions.value, while > interesting, makes me a little nervous. I see how it makes some things > more compact and allows you to combine a select and keyword model > easily. But it also feels a bit fragile, and different from all the > other menus in mutt. I'm not vehemently opposed, but was just curious > why you chose that model here. I don't know of any risk to this, though I'm open to believing there is one. I did it because I generally dislike writing multiple tabulations of essentially the same sequence, that must be in sync to avoid errors. I've seen a lot of bugs in large programs that happen because someone didn't know about one of these, so I tend to favor letting the underlying code be smarter about it. It lets others contribute with less entry overhead. Anyone else have remarks on this? > 5. Feel free to add a copyright line in listmenu.c (and also put > yourself in the COPYRIGHT file!) Eh, I've never been too concerned about overtly claiming copyright, but I probaby should after all this time. :) I'm travelling so it will be a little while before I can revise this, but I will. Thanks again! -- David Champion • [email protected]
signature.asc
Description: PGP signature
