-
Notifications
You must be signed in to change notification settings - Fork 166
[RFC] Colour quoted text #1441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[RFC] Colour quoted text #1441
Changes from all commits
de47ee6
9742d11
b94861c
564e598
612d590
238b60b
4f1fd0e
5e2a6f3
c4a480d
2e39381
538278d
32cebe8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| # Copyright (C) 2011-2020 Patrick Totzke <patricktotzke@gmail.com> | ||
| # Copyright © 2019-2020 Chloé Dequeker <contact@nelyah.eu> | ||
| # This file is released under the GNU GPL, version 3 or a later revision. | ||
| # For further details see the COPYING file | ||
| import logging | ||
|
Nelyah marked this conversation as resolved.
|
||
| import re | ||
|
|
||
| from urwid import AttrSpec | ||
| from ..settings.const import settings | ||
|
|
||
|
|
||
| def parse_text_colour(line): | ||
| """Get colour attribute for the line | ||
|
|
||
| :param str line: line of text to be parsed | ||
| :return: The theme attribute to apply | ||
| """ | ||
| if settings.get('parse_quotes'): | ||
|
Nelyah marked this conversation as resolved.
|
||
| return parse_quotes(line) | ||
| else: | ||
| return None | ||
|
|
||
|
|
||
| def parse_quotes(line): | ||
| """Search for quotes. | ||
| Only search up to the 7th quote level. | ||
|
|
||
| :param str line: The line of text to be parsed | ||
| :return: Theming attribute, None if no quote are available | ||
| """ | ||
|
|
||
| # The value is arbitrarily set because we need to define | ||
| # the corresponding attributes in the theming configuration spec. | ||
| max_quote_level = 7 | ||
| quote_colour = get_quote_colour(line, max_quote_level) | ||
|
|
||
| if isinstance(quote_colour, AttrSpec): | ||
| return quote_colour | ||
| else: | ||
| return None | ||
|
|
||
|
|
||
| def get_quote_colour(line, max_quote_level): | ||
|
lucc marked this conversation as resolved.
|
||
| """Cycle through quotation levels for the line | ||
|
|
||
| :param str line: The line of text to be parsed | ||
| :param int max_quote_level: Search for quotes up to that level | ||
| :return: quote_colour (either string 'default' or an AttrSpec object) | ||
| """ | ||
| symbol = settings.get('quote_symbol') | ||
| quote_colour = None | ||
|
|
||
| for quote_level in range(1, max_quote_level+1): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Plus if we remove the |
||
| quote_regex = r'^ *({} *){{{}}}'.format(symbol, quote_level) | ||
| if re.match(quote_regex, line): | ||
| logging.debug( | ||
| 'Requesting attribute quote_level_{}'.format(quote_level)) | ||
| quote_colour = settings.get_theming_attribute( | ||
| 'thread', 'quote_level_{}'.format(quote_level)) | ||
|
|
||
| else: | ||
| # If there is no match at some point, | ||
| # we simply use the last level match colour | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think cycling back to the colours for the first level of quoting is a better option:
etc. Basically a modulo. |
||
| break | ||
| return quote_colour | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the existing
quote_prefix?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see it before!
However, I notice that it has a space added to it. That would mean that a line like:
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_prefixis used. A first solution would be to remove the space and accommodate for this wherever it is used. A second could be to rename myquote_symbolto something more explicit, likequotation_regex_detection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]forquote_symbol.