Skip to content

MessageBox: Extract in-line message links from the message content and show them as footlinks #703

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

MessageBox: Extract in-line message links from the message content and show them as footlinks #703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f795fad
boxes: Add footlinks to improve message legibility in MessageBox.
preetmishra Jul 7, 2020
fae4695
boxes/themes: Add msg_link_index style for link indexes in MessageBox.
preetmishra Jul 9, 2020
960dd2d
boxes: Improve in-line message link text for message legibility.
preetmishra Jul 11, 2020
49ecf51
boxes: Skip a footlink its text is sufficient to represent the link.
preetmishra Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 100 additions & 7 deletions tests/ui/test_ui_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,6 +1473,7 @@ def test_init(self, mocker, message_type, set_fields):
assert msg_box.last_message == defaultdict(dict)
for field, invalid_default in set_fields:
assert getattr(msg_box, field) != invalid_default
assert msg_box.message_links == OrderedDict()

def test_init_fails_with_bad_message_type(self):
message = dict(type='BLAH')
Expand Down Expand Up @@ -1515,17 +1516,48 @@ def test_private_message_to_self(self, mocker):
('<blockquote>stuff', [('msg_quote', ['', 'stuff'])]),
('<div class="message_embed">',
['[EMBEDDED CONTENT NOT RENDERED]']), # FIXME Unsupported
('<a href="http://foo">http://foo</a>', [('msg_link', 'http://foo')]),
# TODO: Generate test cases to work with both soup2markup and
# footlinks_view.
('<a href="http://foo">Foo</a><a href="https://bar.org">Bar</a>',
[('msg_link', 'Foo'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'Bar'), ' ', ('msg_link_index', '[2]')]),
('<a href="http://foo">Foo</a><a href="http://foo">Another foo</a>',
[('msg_link', 'Foo'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'Another foo'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo">Foo</a><a href="https://bar.org">Bar</a>'
'<a href="http://foo">Foo</a><a href="https://bar.org">Bar</a>',
[('msg_link', 'Foo'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'Bar'), ' ', ('msg_link_index', '[2]'),
('msg_link', 'Foo'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'Bar'), ' ', ('msg_link_index', '[2]')]),
('<a href="http://baz.com/">http://baz.com/</a>',
[('msg_link', 'http://baz.com'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo.com/">Foo</a><a href="http://foo.com">Foo</a>',
[('msg_link', 'Foo'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'Foo'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo">http://foo</a>',
[('msg_link', 'http://foo'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo/bar.png">http://foo/bar.png</a>',
[('msg_link', 'http://foo/bar.png')]),
('<a href="http://foo">bar</a>', [('msg_link', '[bar](http://foo)')]),
[('msg_link', 'bar.png'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo">bar</a>',
[('msg_link', 'bar'), ' ', ('msg_link_index', '[1]')]),
('<a href="/user_uploads/blah"',
[('msg_link', '[]({}/user_uploads/blah)'.format(SERVER_URL))]),
[('msg_link', 'blah'), ' ', ('msg_link_index', '[1]')]),
('<a href="/api"',
[('msg_link', '[]({}/api)'.format(SERVER_URL))]),
[('msg_link', 'api'), ' ', ('msg_link_index', '[1]')]),
('<a href="some/relative_url">{}/some/relative_url</a>'
.format(SERVER_URL),
[('msg_link', '{}/some/relative_url'.format(SERVER_URL))]),
[('msg_link', 'relative_url'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo.com/bar">foo.com/bar</a>',
[('msg_link', 'bar'), ' ', ('msg_link_index', '[1]')]),
('<a href="http://foo.com">foo.com</a>'
'<a href="http://foo.com">http://foo.com</a>'
'<a href="https://foo.com">https://foo.com</a>'
'<a href="http://foo.com">Text</a>',
[('msg_link', 'foo.com'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'http://foo.com'), ' ', ('msg_link_index', '[1]'),
('msg_link', 'https://foo.com'), ' ', ('msg_link_index', '[2]'),
('msg_link', 'Text'), ' ', ('msg_link_index', '[1]')]),
('<li>Something', ['\n', ' \N{BULLET} ', '', 'Something']),
('<li></li>', ['\n', ' \N{BULLET} ', '']),
('<li>\n<p>Something',
Expand Down Expand Up @@ -1624,8 +1656,11 @@ def test_private_message_to_self(self, mocker):
'empty', 'p', 'user-mention', 'group-mention', 'code', 'codehilite',
'strong', 'em', 'blockquote',
'embedded_content',
'link_two', 'link_samelinkdifferentname', 'link_duplicatelink',
'link_trailingslash', 'link_trailingslashduplicatelink',
'link_sametext', 'link_sameimage', 'link_differenttext',
'link_userupload', 'link_api', 'link_serverrelative_same',
'link_textwithoutscheme', 'link_differentscheme',
'li', 'empty_li', 'li_with_li_p_newline', 'two_li',
'two_li_with_li_p_newlines', 'li_nested', 'li_heavily_nested',
'br', 'br2', 'hr', 'hr2', 'img', 'img2',
Expand Down Expand Up @@ -2047,7 +2082,7 @@ def test_keypress_EDIT_MESSAGE(self, mocker, message_fixture,
<p>C</p>""", "░ A\n░ B\n\nC"),
("""<blockquote>
<p><a href='https://chat.zulip.org/'</a>czo</p>
</blockquote>""", "░ [czo](https://chat.zulip.org/)\n"),
</blockquote>""", "░ czo [1]\n"),
pytest.param("""<blockquote>
<blockquote>
<p>A<br>
Expand Down Expand Up @@ -2142,6 +2177,64 @@ def test_reactions_view(self, message_fixture, to_vary_in_each_message):
('reaction_mine', 9),
]

@pytest.mark.parametrize(['message_links', 'expected_text',
'expected_attrib'], [
(OrderedDict([
('https://github.com/zulip/zulip-terminal/pull/1', ('#T1', 1,
True)),
]),
'1: https://github.com/zulip/zulip-terminal/pull/1',
[('msg_link_index', 2), (None, 1), ('msg_link', 46)]),
(OrderedDict([
('https://foo.com', ('Foo!', 1, True)),
('https://bar.com', ('Bar!', 2, True)),
]),
'1: https://foo.com\n2: https://bar.com',
[('msg_link_index', 2), (None, 1), ('msg_link', 15), (None, 1),
('msg_link_index', 2), (None, 1), ('msg_link', 15)]),
(OrderedDict([
('https://example.com', ('https://example.com', 1, False)),
('http://example.com', ('http://example.com', 2, False)),
]),
None,
None),
(OrderedDict([
('https://foo.com', ('https://foo.com, Text', 1, True)),
('https://bar.com', ('Text, https://bar.com', 2, True)),
]),
'1: https://foo.com\n2: https://bar.com',
[('msg_link_index', 2), (None, 1), ('msg_link', 15), (None, 1),
('msg_link_index', 2), (None, 1), ('msg_link', 15)]),
(OrderedDict([
('https://foo.com', ('Foo!', 1, True)),
('http://example.com', ('example.com', 2, False)),
('https://bar.com', ('Bar!', 3, True)),
]),
'1: https://foo.com\n3: https://bar.com',
[('msg_link_index', 2), (None, 1), ('msg_link', 15), (None, 1),
('msg_link_index', 2), (None, 1), ('msg_link', 15)]),
],
ids=[
'one_footlink',
'more_than_one_footlink',
'similar_link_and_text',
'different_link_and_text',
'http_default_scheme',
]
)
def test_footlinks_view(self, message_fixture, message_links,
expected_text, expected_attrib):
msg_box = MessageBox(message_fixture, self.model, None)

footlinks = msg_box.footlinks_view(message_links)

if expected_text:
assert footlinks.original_widget.text == expected_text
assert footlinks.original_widget.attrib == expected_attrib
else:
assert footlinks is None
assert not hasattr(footlinks, 'original_widget')

@pytest.mark.parametrize(
'key', keys_for_command('ENTER'),
ids=lambda param: 'left_click-key:{}'.format(param)
Expand Down
8 changes: 8 additions & 0 deletions zulipterminal/config/themes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
'reaction_mine': 'standout',
'msg_mention': 'bold',
'msg_link': '',
'msg_link_index': 'bold',
'msg_quote': 'underline',
'msg_code': 'bold',
'msg_bold': 'bold',
Expand Down Expand Up @@ -69,6 +70,7 @@
DARKBLUE = 'h24' # faded_blue
DARKRED = 'h88' # faded_red
LIGHTBLUE = 'h109' # bright_blue
LIGHTBLUEBOLD = '%s, bold' % LIGHTBLUE
YELLOW = 'h172' # neutral_yellow
YELLOWBOLD = '%s, bold' % YELLOW
LIGHTGREEN = 'h142' # bright_green
Expand Down Expand Up @@ -129,6 +131,8 @@
None, DEF['light_red:bold'], DEF['black']),
('msg_link', 'light blue', 'black',
None, DEF['light_blue'], DEF['black']),
('msg_link_index', 'light blue, bold', 'black',
None, DEF['light_blue:bold'], DEF['black']),
('msg_quote', 'brown', 'black',
None, DEF['brown'], DEF['black']),
('msg_code', 'black', 'white',
Expand Down Expand Up @@ -193,6 +197,8 @@
None, LIGHTREDBOLD, BLACK),
('msg_link', 'light blue', 'black',
None, LIGHTBLUE, BLACK),
('msg_link_index', 'light blue, bold', 'black',
None, LIGHTBLUEBOLD, BLACK),
('msg_quote', 'brown', 'black',
None, 'brown', BLACK),
('msg_code', 'black', 'white',
Expand Down Expand Up @@ -234,6 +240,7 @@
('reaction_mine', 'white', 'light magenta'),
('msg_mention', 'light red, bold', 'white'),
('msg_link', 'dark blue', 'white'),
('msg_link_index', 'dark blue, bold', 'white'),
('msg_quote', 'black', 'brown'),
('msg_code', 'black', 'light gray'),
('msg_bold', 'white, bold', 'dark gray'),
Expand Down Expand Up @@ -266,6 +273,7 @@
('reaction_mine', 'light blue', 'dark magenta'),
('msg_mention', 'light red, bold', 'light blue'),
('msg_link', 'dark blue', 'light gray'),
('msg_link_index', 'dark blue, bold', 'light gray'),
('msg_quote', 'brown', 'dark blue'),
('msg_code', 'dark blue', 'white'),
('msg_bold', 'white, bold', 'dark blue'),
Expand Down
91 changes: 83 additions & 8 deletions zulipterminal/ui_tools/boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ def __init__(self, message: Message, model: Any,
self.topic_name = ''
self.email = ''
self.user_id = None # type: Union[int, None]
self.message_links = (
OrderedDict()
) # type: OrderedDict[str, Tuple[str, int, bool]]
self.last_message = last_message
# if this is the first message
if self.last_message is None:
Expand Down Expand Up @@ -402,6 +405,31 @@ def reactions_view(self, reactions: List[Dict[str, Any]]) -> Any:
except Exception:
return ''

# Use quotes as a workaround for OrderedDict typing issue.
# See https://github.com/python/mypy/issues/6904.
def footlinks_view(
self, message_links: 'OrderedDict[str, Tuple[str, int, bool]]',
) -> Any:
footlinks = []
for link, (text, index, show_footlink) in message_links.items():
if not show_footlink:
continue

footlinks.extend([
('msg_link_index', '{}:'.format(index)),
' ',
('msg_link', link),
'\n',
])

if not footlinks:
return None

footlinks[-1] = footlinks[-1][:-1] # Remove the last newline.
return urwid.Padding(urwid.Text(footlinks),
align='left', left=8, width=('relative', 100),
min_width=10, right=2)

def soup2markup(self, soup: Any, **state: Any) -> List[Any]:
# Ensure a string is provided, in case the soup finds none
# This could occur if eg. an image is removed or not shown
Expand Down Expand Up @@ -468,22 +496,65 @@ def soup2markup(self, soup: Any, **state: Any) -> List[Any]:
markup.append(('msg_mention', element.text))
elif element.name == 'a':
# LINKS
link = element.attrs['href']
# Use rstrip to avoid anomalies and edge cases like
# https://google.com vs https://google.com/.
link = element.attrs['href'].rstrip('/')
text = element.img['src'] if element.img else element.text
text = text.rstrip('/')

parsed_link = urlparse(link)
if not parsed_link.scheme: # => relative link
# Prepend org url to convert it to an absolute link
link = urljoin(self.model.server_url, link)

if link == text:
# If the link and text are same
# usually the case when user just pastes
# a link then just display the link
markup.append(('msg_link', text))
text = text if text else link

show_footlink = True
# Only use the last segment if the text is redundant.
# NOTE: The 'without scheme' excerpt is to deal with the case
# where a user puts a link without any scheme and the server
# uses http as the default scheme but keeps the text as-is.
# For instance, see how example.com/some/path becomes
# <a href="http://example.com">example.com/some/path</a>.
link_without_scheme, text_without_scheme = [
data.split('://')[1] if '://' in data else data
for data in [link, text]
] # Split on '://' is for cases where text == link.
if link_without_scheme == text_without_scheme:
segment = text.split('/')[-1]
# Replace text with its last segment if the segment has
# something significant than simply the 'domain name'.
if segment != text_without_scheme:
text = segment
else:
# Do not show as a footlink as the text is sufficient
# to represent the link.
show_footlink = False

# Detect duplicate links to save screen real estate.
if link not in self.message_links:
self.message_links[link] = (
text, len(self.message_links) + 1, show_footlink
)
else:
markup.append(
('msg_link', '[' + text + ']' + '(' + link + ')'))
# Append the text if its link already exist with a
# different text.
saved_text, saved_link_index, saved_footlink_status = (
self.message_links[link]
)
if saved_text != text:
self.message_links[link] = (
'{}, {}'.format(saved_text, text),
saved_link_index,
show_footlink or saved_footlink_status,
)

markup.extend([
('msg_link', text),
' ',
('msg_link_index',
'[{}]'.format(self.message_links[link][1])),
])
elif element.name == 'blockquote':
# BLOCKQUOTE TEXT
markup.append((
Expand Down Expand Up @@ -660,11 +731,15 @@ def main_view(self) -> List[Any]:
# Reactions
reactions = self.reactions_view(self.message['reactions'])

# Footlinks.
footlinks = self.footlinks_view(self.message_links)

# Build parts together and return
parts = [
(recipient_header, recipient_header is not None),
(content_header, any_differences),
(content, True),
(footlinks, footlinks is not None),
(reactions, reactions != ''),
]
return [part for part, condition in parts if condition]
Expand Down