pazz / alot

Terminal-based Mail User Agent
GNU General Public License v3.0
697 stars 165 forks source link

Message summary subjects #1589

Open kbingham opened 2 years ago

kbingham commented 2 years ago

This series tackles presenting threads where a multi-patch series is sent with a cover letter, for instance when sent with git-send-mail.

The default presentation in the thread view hides the details required to identify the patch titles: alot-without-message-summaries

With this development the individual patches can be seen to show their titles on the summary.

alot-with-message-summaries

To prevent adding a lot of noise, replies do not further repeat the message title. The implemenation here so far is just to check if 'subject.startswith(('Re:')) and ignore those ones which does not seem fool-proof (and may be entirely locale dependant based upon the senders) so I'm looking for advice on a better way to implement that.

I had wondered if comparing the Subject against a parent subject, and only printing if they differ would be better, but the prefixes that get added by replies would still affect that too.

kbingham commented 2 years ago

Just discovered: https://github.com/mjg/alot/commit/f788ddd0659ae6468871b225f2bd8160a5c656d7 Which looks like a better way to implement the get_subject() perhaps.

pazz commented 2 years ago

I'm not sure which implementation of "get_subject" is better; perhaps @mjg has an opinion. I am actually surprised that this isn't already there (is that patch you mention not in master?).

Ideally, the message summary would be configurable in the theme just like threadline is in search mode. At the very least I believe we should have config settings to enable/disable whether subjects are displayed at all and if so, if they are re-interpreted as you suggest.

kbingham commented 2 years ago

I'm not running master yet, still on 0.9.1 as I couldn't (yet) figure out how to get the notmuch2 bindnigs to work. But probably time to retry that and get back on master.

That said, no @mjg 's patch isn't on master, probably for the same reasons you've highlighted above ;-) (the get_subject and summary line is all in one patch currently.).

Once I'm on master, I'll see about sending a PR for just the get_subject() as that is likely to be useful on it's own anyway, and then tackle the configuration file parts.

mjg commented 2 years ago

I use alot with a couple of private additions, and per-message-subject is one of them. Similarly, unset hide_thread_subject was one of my first settings when I (partially) switched to (neo)mutt.

I don't think matching for Re: is a good way forward: It depends on the sender's locale and it wrongly matches on changed subjects such as Re: Your subject IMPORTANT CORRECTION. Similarly, the option to check whether the subject ends in the original subject fails on IMPORTANT Correction. Re: Your subject and such.

But then I don't know why anyone would want to hide subjects, unless it's on a narrow screen or with deeply nested threads. In other words: I'm the wrong person to ask ;)

Maybe a config which can be bound to a key?

BTW: Some of my old PRs might be stale, i.e. would possibly need rework, especially everything related to "full thread", "requery". Whenever there are recent rebases it means that I'm at least still using them, such as per-message-subject.

mjg commented 2 years ago

... and now I noticed the real content of the question...

So, deferring decode_header() like @kbingham does is certainly a good idea unless you expect to call get_subject() anyways (which i did). OTOH one should probably catch the exceptions like in my patch. Other than that we're doing the same thing here, aren't we?

kbingham commented 2 years ago

More or less the same thing yes. And how to determine when to trim(stop adding) the subject is the hard part that I don't yet know ;-)

But I think it's important (to me) as otherwise the screen is just too busy in review conversations: So I think this:

alot-with-trimmed-message-summaries

Is much cleaner/more readable than

alot-without-trimmed-message-summaries

I think I'd like to figure out how to change the colour/highlight of the subject too, like it does on the main search view so that we can quickly differentiate between the sender/datestamp/subject on the summary line.

More for me to experiment and play with I guess.

pazz, do you still use the IRC channel? or just lurk - I've asked a question there about trying to run on master, so I can't rebase these to latest yet.

mjg commented 2 years ago

The thing is that git send-email and some people do the "wrong thing": they specify both In-Reply-To and References when indeed the patches are not a reply to the cover letter, for example. OTOH I don't think there's a widely accepted standard for saying "related but not a reply".

Just for fun: A bank which I sent an e-mail "Re: some topic" to apparantly translated "Re" into "Reklamation" (complaint) automatically and redirected the e-mail to that department...

So, I think your best option to check whether e-mail 2 is a "real reply" to e-mail 1 is by checking whether e-mail 1's subject is contained in that of e-mail 2 (plus maybe a safety check on difference of length being smallish). This catches all kinds of "Re/RE/AW", and forwards typically do not set the headers, so they're not in the mix anyways.

For an extra safety measure, replace the matched subject by ..., so that message 2 would display the subject as Re: ... or AW: ... or Why do you write an e-mail with subject ... to me? which loses none of the extra information but still is much shorter than the full subject. I think even I would like that.

Or let the user define a hook or regex for the match.

kbingham commented 2 years ago

Oh, I really like the 'replace' with '...' and that solves a lot of the concerns anyway, and provides the same end goal.

kbingham commented 2 years ago

@mjg The '...' looks nice and seems to solve it so far. I'll keep testing this locally for a bit and fix up the warning, and some more cleanups.

I still need some help getting the master branch to run before I can base this on top of 0.10 though. It seems like the failure starts after the notmuch2 API change, but as far as I can tell I have the notmuch2 bindings installed.

kbingham commented 2 years ago

@mjg Thanks for that suggestion, it's working really well so far, and helps highlight differences in the subject from the parent quite nicely.

alot-with-message-summaries-replace

There are a few 'exceptions' I might want to look into (mostly so far between case sensitivity of Re: vs RE: ) - but it's handling the main use cases really well.

mjg commented 2 years ago

Interesting. I think what I meant was slightly different, but both works. The differece is in whether you replace a match for the original subject (if it matches) or for the previous subject, or in other words: When does the meaning of ... change? Given:

Really important topic
+Re: Really important topic
++Re: Really important topic
+++AW: Re: Really important topic
++++Re: AW: Re: Really important topic
+++++Re: AW: Re: Really important topic

you translate this (if I'm not mistaken) into

Really important topic
+Re: ...
++...
+++AW: ...
++++Re: ...
+++++...

and I was more thinking along the lines of:

Really important topic
+Re: ...
++Re: ...
+++AW: Re: ...
++++Re: AW: Re: ...
+++++Re: AW: Re:...

I.e. keep the meaning of ... as long as it matches. I haven't looked at the code, though.

kbingham commented 2 years ago

The difficulty is that I can /only/ match against a single parent. I can't match that 'all the way up'. Otherwise that would only ever consider the top level thread subject.

But more or less, I'm simply replacing the parent subject in the current subject .. So this indeed would be:

Really important topic
+Re: ...        ### Matches top thread
++ ...          ### No additional Re: so matches the above and all removed.
+++AW: ...      ### Only the AW: is 'new' and visible.
++++Re: ...     ### The AW was in parent, and now also removed.
+++++...        ### Entire subject is the same as parent.

To keep all prefixes we'd have to be able to know that we could selectively remove 'standard' prefixes from the match criteria, which might be reasonable to do.

We could 'strip' the list of prefixes (https://en.wikipedia.org/wiki/List_of_email_subject_abbreviations) before making the match/compares?

kbingham commented 2 years ago

To clarify in: alot-with-message-summaries-replace

If I always match only the top level message summary - It would always try to replace "[PATCH 00/11] Document all the IPU3 IPA classes" which isn't actually replied to at all in the entire thread.

Here's the raw subjects for the first few lines of the image above:

[PATCH 00/11] Document all the IPU3 IPA classes
-> [PATCH 01/11] ipa: ipu3: Document IPAIPU3 class interface
-> -> Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3 class interface
-> -> -> Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3 class interface
-> -> -> Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3 class interface
-> -> -> -> Re: [libcamera-devel] [PATCH 01/11] ipa: ipu3: Document IPAIPU3 class interface
-> [libcamera-devel] [PATCH 02/11] ipa: ipu3: Improve the documentation of BDS grid 
-> -> Re: [libcamera-devel] [PATCH 02/11] ipa: ipu3: Improve the documentation of BDS grid

Which currently generates:

[PATCH 00/11] Document all the IPU3 IPA classes
-> [PATCH 01/11] ipa: ipu3: Document IPAIPU3 class interface
-> -> Re: [libcamera-devel] ...
-> -> -> ...
-> -> -> ...
-> -> -> -> ...
-> [libcamera-devel] [PATCH 02/11] ipa: ipu3: Improve the documentation of BDS grid 
-> -> Re: ...

Note how the [libcamera-devel] is only present on patches that I received through the list, and differs to the subject of the mail that I may receive if the mail comes directly to me. It might be a race or specific to whether I was on direct CC/To as to which one gets used. I usually interpret mails that don't have the list on as being sent 'specifically' to me.

mjg commented 2 years ago

Sure, what I meant is: Starting at root, set threadsubject = subject of root. Then (pseudocode) for each child repeat:

if threadsubject in currentsubject:
    replace threadsubject by ... in currentsubject for display
else:
   threadsubject = currentsubject # for the subthread beginning here

As I wrote, it depends on how you actually interpret .... In your algorithm, messages with the same subject end up being represented differently: Re: ... vs. ... for the first reply vs. the reply to that. Now, the first reply clearly changed the subject (prepending Re:) while the reply to that did not, so I guess by looking at it that way everything is fine with your approach.

kbingham commented 2 years ago

Sure, what I meant is: Starting at root, set threadsubject = subject of root. Then (pseudocode) for each child repeat:

if threadsubject in currentsubject:
    replace threadsubject by ... in currentsubject for display
else:
   threadsubject = currentsubject # for the subthread beginning here

As I wrote, it depends on how you actually interpret .... In your algorithm, messages with the same subject end up being represented differently: Re: ... vs. ... for the first reply vs. the reply to that. Now, the first reply clearly changed the subject (prepending Re:) while the reply to that did not, so I guess by looking at it that way everything is fine with your approach.

Aha, ok that makes it clearer what you were intending actually, thanks. I'm not yet sure how easy it would be to implement that in the widget, I'm referencing 'up' by checking the parent of each summary, rather than iterating down ...

(Well, the iteration down happens when creating the Messages, but I don't think we should be trailing other message's subjects around with a Message object).

kbingham commented 2 years ago

Now that I've fixed alot to run on master with my config, I've rebased this. Probably still a bit more refinement needed, but this really helps my work-load a lot.

kbingham commented 2 years ago

Small updates here. The method of getting the subject has been simplified, as the subject headers are indexed by not-much - so from my understanding it's more efficient to simply get it when constructing the Message object, and retain it at that point.

I've also picked up a fix in this series, while testing as I sent myself a mail with no subject to validate this. And that hit a failure in existing code paths. So that's included as the last patch.

Open questions are, if this implementation for stripping the parent subject is satisfactory to all, and if this should be any kind of a config option... or if this should be default behaviour.