[RFC] Colour quoted text #1441

Nelyah · 2019-10-10T14:25:15Z

Context

I feel like being able to render quote text according to the quoted level with colours is important. At least it is for me in terms of readability.

This PR is to answer this particular feature. An issue has been opened #746, but it was answered with #612 and then #1015. These are great features, but don't quite target this specific issue.

Modifications

I modified the file alot/widget/thread.py to call the parse_text_colour function, if we are looking at words linewise. The function parse_text_colour asks the config to check if we want to parse quoted text. If yes, it proceeds and calls the function parse_quotes.

The latter returns the theme value associated with the quote_level in the theme file (if there is one).

Example configuration

This is working well for me and it passes the tests. This is how I set it up in my config file to make either > or four spaces a quote symbol.:

quote_symbol = '( {4}|>)'
parse_quotes = True

I am fairly new to this project, please let me know if there are obvious things I am missing.

pazz · 2019-10-10T15:06:28Z

Welcome to alot @Nelyah, and thanks for starting to contribute right away!
Would you mind sharing a screenshot so that we can better understand what exactly the feature is and which the consequences of the code changes are?

Would this only change the urwid representation or does your code changes the text of follow-up reply emails?

Nelyah · 2019-10-10T22:23:32Z

Thank you @pazz, this is a wonderful piece of software you got there!

Here is a screenshot of a sample mail. I set up the colours being close from one another, but this can be changed with the options quote_level_i in the configuration file (where i is the quotation level wished for). I declared those only up to i=7. Let me know if you'd wish more colour contrast.

Would this only change the urwid representation or does your code changes the text of follow-up reply emails?

No, this should only change the urwid representation. The only real modification to existing code is to change the attr theming value (in alot/widget/thread.py). I am getting the new attr value by parsing the mail body and searching it for quotation markers (which can be defined in the configuration file)

alot/widgets/colour_text.py

Nelyah · 2019-10-11T16:15:35Z

I just added the tests as well

tests/widgets/test_colour_text.py

Nelyah · 2019-10-11T22:05:24Z

Thank you for the feedback, @lucc! And sorry if this has taken some time and back and forth.

pazz

Can you please squash down those commits that make trivial changes or revert stuff you introduced yourself earlier? I started to review the commits one by one and realized that some of my (line) comments are already addressed in later commits...

alot/widgets/colour_text.py

pazz · 2019-11-03T14:38:37Z

alot/widgets/colour_text.py

+    is_quoted = False
+    quote_colour = None
+    symbol = settings.get('quote_symbol')
+    if symbol is None:


use the fallback option when getting the config setting? https://github.com/pazz/alot/blob/master/alot/settings/manager.py#L239
(note that the defaul is "> ", with a trailing space)

Does it make sense here to specify a fallback? If I understand correctly we would still get the default settings value of the variable we're requesting (in this case from quote_symbol : >), even if no fallback is defined.

alot/widgets/colour_text.py

pazz · 2019-11-03T14:43:06Z

alot/defaults/alot.rc.spec

@@ -17,6 +17,10 @@ input_timeout = float(default=1.0)
 # .. note:: this config setting is equivalent to, but independent of, the 'search.exclude_tags' in the notmuch config.
 exclude_tags = force_list(default=list())

+# Show quotes be parsed
+parse_quotes = boolean(default=True)
+quote_symbol = string(default='>')


why not use the existing quote_prefix?

I didn't see it before!

However, I notice that it has a space added to it. That would mean that a line like:

>> Level 2 quote level

wouldn't get interpreted correctly, because the regex r'^ *({} *){{{}}}'.format(symbol, quote_level) wouldn't match. It would still be possible to change it but the default wouldn't work really well.

I'm not sure in what context quote_prefix is used. A first solution would be to remove the space and accommodate for this wherever it is used. A second could be to rename my quote_symbol to something more explicit, like quotation_regex_detection.

I don't think this new option is necessary. The two most commonly used characters for quoting text are > and |, and I've only ever seen the bracket being used (wikipedia reference).

It would make the PR leaner to assume only those two characters highlight quotes in messages, plus it would avoid some weird unit tests verifying that the code behaves as expected when using [Aa] for quote_symbol.

Nelyah · 2019-11-03T16:13:47Z

I'll do the licence change and add the quote_prefix (didn't see it previously).
However squashing commits (which I have already pushed to origin) through a rebase means I am rewriting all of my commits as my new master diverges from origin/master, isn't it?
I'm not quite a git professional, but let me know if this is what I should do (unless there is a better solution for it)

lucc · 2019-11-04T06:42:59Z

You can do an interactive rebase. Like this git rebase --interactive origin/master message-body-colour. Learn more about it from the docs.

pazz · 2020-05-06T09:22:52Z

Where are we on this? It'd be a shame to let this get too old and diverge from master too much to be useful..

Nelyah · 2020-05-06T09:53:38Z

Oh, I'm sorry.

I had some things happening at the time and it just slipped out of my mind. I will get back to this in the next few days! Sorry again for the delay...

issue pazz#1502

Up to quote_level 7

Also generate the docs for those settings

As of now, only the function parse_quotes is implemented, but other ideas could be added.

… True and quotes were found

Add tests for checking that: - Regex works - Empty quote means we return None - Check we're actually getting the expected attribute

Nelyah · 2020-05-08T19:51:44Z

I rebased and resolved a few merge conflicts (mainly in thread.py with FocusableText being replaced by ANSIText), but have not yet added anything else.

I added a few [tags] to my commits to help myself (and maybe you as well) since it's been a while I worked on this. I'll remove them before merging this.

lenormf · 2020-06-18T13:39:36Z

alot/defaults/alot.rc.spec

@@ -17,6 +17,10 @@ input_timeout = float(default=1.0)
 # .. note:: this config setting is equivalent to, but independent of, the 'search.exclude_tags' in the notmuch config.
 exclude_tags = force_list(default=list())

+# Show quotes be parsed
+parse_quotes = boolean(default=True)
+quote_symbol = string(default='>')


I don't think this new option is necessary. The two most commonly used characters for quoting text are > and |, and I've only ever seen the bracket being used (wikipedia reference).

It would make the PR leaner to assume only those two characters highlight quotes in messages, plus it would avoid some weird unit tests verifying that the code behaves as expected when using [Aa] for quote_symbol.

lenormf · 2020-06-18T13:42:48Z

alot/widgets/colour_text.py

+    symbol = settings.get('quote_symbol')
+    quote_colour = None
+
+    for quote_level in range(1, max_quote_level+1):


I think running 7 regex matches on a line that has 7 levels of nested quoting is too much.

I made it work (with a dirty patch) on my local branch with only two calls to re.match(), for any amount of nesting, I think it's the better way to go about it.

Plus if we remove the quote_symbol option (c.f. my other comment), it will be easier to parse spaces properly (RFC reference)

lenormf · 2020-06-18T13:44:25Z

tests/widgets/test_colour_text.py

+            'colourmode': 256
+            }
+
+SETTINGS_test_symbol_regex = {'quote_symbol': '([Aa]|<>.b|>)',


As mentioned in another comment, this is a weird thing to test for. Maybe we don't need the code to be that generic.

lenormf · 2020-06-18T13:46:20Z

alot/widgets/colour_text.py

+
+        else:
+            # If there is no match at some point,
+            # we simply use the last level match colour


I think cycling back to the colours for the first level of quoting is a better option:

8 → 1

9 → 2

…

15 → 1

16 → 2

etc.

Basically a modulo.

lucc requested changes Oct 11, 2019

View reviewed changes

alot/widgets/colour_text.py Outdated Show resolved Hide resolved

alot/widgets/colour_text.py Outdated Show resolved Hide resolved

alot/widgets/colour_text.py Show resolved Hide resolved

lucc requested changes Oct 11, 2019

View reviewed changes

lucc approved these changes Oct 11, 2019

View reviewed changes

pazz requested changes Nov 3, 2019

View reviewed changes

pazz mentioned this pull request Nov 14, 2019

[RFC] ANSI-escape codes for colourful HTML mails #1015

Merged

pazz added this to the 0.9 milestone Nov 21, 2019

pazz and others added 7 commits May 8, 2020 19:16

overhaul widgets.ANSIText

de47ee6

issue pazz#1502

setting: [Theme] Add new colour values for quoted text

9742d11

Up to quote_level 7

setting: [alot] Declare new settings for parsing quoted text

b94861c

Also generate the docs for those settings

feat: [ColourText] Add a new file to parse a line

564e598

As of now, only the function parse_quotes is implemented, but other ideas could be added.

feat: [ColourText] only return a value if the setting parse_quotes is…

612d590

… True and quotes were found

feat: [thread] Call parse_text_colour on each line when linewise

238b60b

tests: [test/ColourText] Add unit tests for parse_quotes function

4f1fd0e

Add tests for checking that: - Regex works - Empty quote means we return None - Check we're actually getting the expected attribute

Nelyah force-pushed the message-body-colour branch from dec790c to 4f1fd0e Compare May 8, 2020 19:44

format: code climate formating

5e2a6f3

Nelyah added 4 commits May 8, 2020 22:27

fix: [docs] regenerate the docs after modifying a comment

c4a480d

format: [ColourText] Add an else case for readability

2e39381

[ColourText] Add licence for file

538278d

fix: [tests/CT] Remove unused import

32cebe8

Repository owner deleted a comment from Nelyah May 12, 2020

lenormf reviewed Jun 18, 2020

View reviewed changes

lucc removed this from the 0.9 milestone Jul 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Colour quoted text #1441

[RFC] Colour quoted text #1441

Nelyah commented Oct 10, 2019

pazz commented Oct 10, 2019

Nelyah commented Oct 10, 2019

Nelyah commented Oct 11, 2019

Nelyah commented Oct 11, 2019

pazz left a comment

pazz Nov 3, 2019

Nelyah May 8, 2020

pazz Nov 3, 2019

Nelyah May 8, 2020

lenormf Jun 18, 2020

Nelyah commented Nov 3, 2019

lucc commented Nov 4, 2019

pazz commented May 6, 2020

Nelyah commented May 6, 2020

Nelyah commented May 8, 2020

lenormf Jun 18, 2020

lenormf Jun 18, 2020

lenormf Jun 18, 2020

lenormf Jun 18, 2020

[RFC] Colour quoted text #1441

Are you sure you want to change the base?

[RFC] Colour quoted text #1441

Conversation

Nelyah commented Oct 10, 2019

Context

Modifications

Example configuration

pazz commented Oct 10, 2019

Nelyah commented Oct 10, 2019

Nelyah commented Oct 11, 2019

Nelyah commented Oct 11, 2019

pazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nelyah commented Nov 3, 2019

lucc commented Nov 4, 2019

pazz commented May 6, 2020

Nelyah commented May 6, 2020

Nelyah commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment