VM

Code review comment for lp:~akwm/vm/abbreviate-headers

Revision history for this message
Uday Reddy (reddyuday) wrote :

Hi Arik, here are my comments on the abbreviate-headers branch. (sorry for
the delay in reviewing this, due to some pre-arranged travel.)

- vm-fill-headers-column - the defcustom :type should be something like
  '(choice (const nil) integer)

- The regexp used for searching for header fields doesn't work for headers
  that have hyphens, e.g., X-Mailer. It must conform to the RFC
  definition of header fields. Check the function vm-match-header in
  vm-folder.el.

- Add a space between the header text and the "[...]" button to make it look
  cleaner. Can the button use the 'vm-mime-button-face? If so, you can
  shorten the button-text as well.

- The "[...]" button is a bit confusing because it doesn't indicate the
  current state of the abbreviation. It could be something like "[>>>]" for
  an abbreviated header and "[<<<]" for an expanded header.

- If this is going to replace rfaddons' shrunken-headers feature, then it is
  best to use the same naming, i.e., "shrunken" headers instead of "shrunk"
  headers or "abbreviated" headers. Differences in terminology can cause
  confusion down the road.

- The vm-fill-headers function seems a bit fiddly in engineering the
  correct indentation for the continuation lines. Can't the fill-prefix be
  used for this purpose, more cleanly?

- VM source files use a tab-width of 8 columns. So, please set your
  tab-width to 8 or turn off tabs.

Cheers,
Uday

« Back to merge proposal