Skip to content

Commit

Permalink
boxes: Add footlinks to improve message legibility in MessageBox.
Browse files Browse the repository at this point in the history 
This indexes in-line message links and adds footlinks to avoid
disrupting the flow of [reading] a message.

Additionally, message_links is added as an instance attribute to store
links and their metadata.

Test added and amended.

Fixes zulip#618.
  • Loading branch information
@preetmishra
preetmishra committed Jul 15, 2020
1 parent f4dc819 commit f795fad
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 15 deletions.
91 changes: 84 additions & 7 deletions tests/ui/test_ui_tools.py
View file
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,40 @@ 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'), ' ', '[1]',
('msg_link', 'Bar'), ' ', '[2]']),
('<a href="http://foo">Foo</a><a href="http://foo">Another foo</a>',
[('msg_link', 'Foo'), ' ', '[1]',
('msg_link', 'Another foo'), ' ', '[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'), ' ', '[1]',
('msg_link', 'Bar'), ' ', '[2]',
('msg_link', 'Foo'), ' ', '[1]',
('msg_link', 'Bar'), ' ', '[2]']),
('<a href="http://baz.com/">http://baz.com/</a>',
[('msg_link', 'http://baz.com'), ' ', '[1]']),
('<a href="http://foo.com/">Foo</a><a href="http://foo.com">Foo</a>',
[('msg_link', 'Foo'), ' ', '[1]',
('msg_link', 'Foo'), ' ', '[1]']),
('<a href="http://foo">http://foo</a>',
[('msg_link', 'http://foo'), ' ', '[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', 'http://foo/bar.png'), ' ', '[1]']),
('<a href="http://foo">bar</a>',
[('msg_link', 'bar'), ' ', '[1]']),
('<a href="/user_uploads/blah"',
[('msg_link', '[]({}/user_uploads/blah)'.format(SERVER_URL))]),
[('msg_link', '{}/user_uploads/blah'.format(SERVER_URL)), ' ',
'[1]']),
('<a href="/api"',
[('msg_link', '[]({}/api)'.format(SERVER_URL))]),
[('msg_link', '{}/api'.format(SERVER_URL)), ' ', '[1]']),
('<a href="some/relative_url">{}/some/relative_url</a>'
.format(SERVER_URL),
[('msg_link', '{}/some/relative_url'.format(SERVER_URL))]),
[('msg_link', '{}/some/relative_url'.format(SERVER_URL)), ' ',
'[1]']),
('<li>Something', ['\n', ' \N{BULLET} ', '', 'Something']),
('<li></li>', ['\n', ' \N{BULLET} ', '']),
('<li>\n<p>Something',
Expand Down Expand Up @@ -1624,6 +1648,8 @@ 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',
'li', 'empty_li', 'li_with_li_p_newline', 'two_li',
Expand Down Expand Up @@ -2047,7 +2073,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 +2168,57 @@ 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)),
]),
'1: https://github.com/zulip/zulip-terminal/pull/1',
[(None, 3), ('msg_link', 46)]),
(OrderedDict([
('https://foo.com', ('Foo!', 1)),
('https://bar.com', ('Bar!', 2)),
]),
'1: https://foo.com\n2: https://bar.com',
[(None, 3), ('msg_link', 15), (None, 4), ('msg_link', 15)]),
(OrderedDict([
('https://example.com', ('https://example.com', 1)),
('http://example.com', ('http://example.com', 2)),
]),
'1: https://example.com\n2: http://example.com',
[(None, 3), ('msg_link', 19), (None, 4), ('msg_link', 18)]),
(OrderedDict([
('https://foo.com', ('https://foo.com, Text', 1)),
('https://bar.com', ('Text, https://bar.com', 2)),
]),
'1: https://foo.com\n2: https://bar.com',
[(None, 3), ('msg_link', 15), (None, 4), ('msg_link', 15)]),
(OrderedDict([
('https://foo.com', ('Foo!', 1)),
('http://example.com', ('example.com', 2)),
('https://bar.com', ('Bar!', 3)),
]),
'1: https://foo.com\n2: http://example.com\n3: https://bar.com',
[(None, 3), ('msg_link', 15), (None, 4), ('msg_link', 18),
(None, 4), ('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)

assert footlinks.original_widget.text == expected_text
assert footlinks.original_widget.attrib == expected_attrib

@pytest.mark.parametrize(
'key', keys_for_command('ENTER'),
ids=lambda param: 'left_click-key:{}'.format(param)
Expand Down
62 changes: 54 additions & 8 deletions zulipterminal/ui_tools/boxes.py
View file
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]]
self.last_message = last_message
# if this is the first message
if self.last_message is None:
Expand Down Expand Up @@ -402,6 +405,28 @@ 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]]',
) -> Any:
footlinks = []
for link, (text, index) in message_links.items():
footlinks.extend([
'{}:'.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 +493,39 @@ 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

# 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
)
else:
markup.append(
('msg_link', '[' + text + ']' + '(' + link + ')'))
# Append the text if its link already exist with a
# different text.
saved_text, saved_link_index = self.message_links[link]
if saved_text != text:
self.message_links[link] = (
'{}, {}'.format(saved_text, text),
saved_link_index,
)

markup.extend([
('msg_link', text),
' ',
'[{}]'.format(self.message_links[link][1]),
])
elif element.name == 'blockquote':
# BLOCKQUOTE TEXT
markup.append((
Expand Down Expand Up @@ -660,11 +702,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

0 comments on commit f795fad

Please sign in to comment.