Skip to content

refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

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

Merged
merged 5 commits into from
Jul 15, 2021
Merged

refactor/bugfix: Migrate keypress to using commands instead of hardcoded keys. #953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
351b45f
refactor: tests: ui_tools: Reduce duplication in mouse scroll tests.
zee-bit Jul 7, 2021
7f29955
tests: ui_tools: Add mouse_event test for TopicsView.
zee-bit Jul 7, 2021
858bf4c
refactor: views: Migrate mouse_events to use primary_key_for_command.
zee-bit Apr 1, 2021
7b11404
bugfix: boxes/views: Make 'r' independent of 'enter' for message_reply.
zee-bit Mar 18, 2021
b810ef7
refactor: ui/views/boxes: Replace hardcoded key in keypress to commands.
zee-bit Mar 17, 2021
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
19 changes: 18 additions & 1 deletion tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import pytest

from zulipterminal.config.keys import keys_for_command
from zulipterminal.config.keys import keys_for_command, primary_key_for_command
from zulipterminal.helper import initial_index as helper_initial_index
from zulipterminal.ui_tools.boxes import MessageBox
from zulipterminal.ui_tools.buttons import StreamButton, TopicButton, UserButton
Expand Down Expand Up @@ -1106,6 +1106,23 @@ def classified_unread_counts():
# --------------- UI Fixtures -----------------------------------------


@pytest.fixture(
params=[
("mouse press", 4, primary_key_for_command("GO_UP")),
("mouse press", 5, primary_key_for_command("GO_DOWN")),
],
ids=[
"mouse_scroll_up",
"mouse_scroll_down",
],
)
def mouse_scroll_event(request):
"""
Returns required parameters for mouse_event keypress
"""
return request.param


@pytest.fixture(
params=[
(key, expected_key)
Expand Down
55 changes: 25 additions & 30 deletions tests/ui/test_ui_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from pytest import param as case
from urwid import Columns, Divider, Padding, Text

from zulipterminal.config.keys import keys_for_command
from zulipterminal.config.keys import keys_for_command, primary_key_for_command
from zulipterminal.config.symbols import (
QUOTED_TEXT_MARKER,
STATUS_ACTIVE,
Expand Down Expand Up @@ -279,14 +279,8 @@ def test_load_new_messages_mocked_log(self, mocker, msg_view, ids_in_narrow):
num_before=0, num_after=30, anchor=0
)

@pytest.mark.parametrize(
"event, button, keypress",
[
("mouse press", 4, "up"),
("mouse press", 5, "down"),
],
)
def test_mouse_event(self, mocker, msg_view, event, button, keypress, widget_size):
def test_mouse_event(self, mocker, msg_view, mouse_scroll_event, widget_size):
event, button, keypress = mouse_scroll_event
mocker.patch.object(msg_view, "keypress")
size = widget_size(msg_view)
msg_view.mouse_event(size, event, button, 0, 0, mocker.Mock())
Expand Down Expand Up @@ -550,19 +544,15 @@ def test_update_streams(self, mocker, stream_view, new_text, expected_log, to_pi
assert hasattr(stream_view.log[0].original_widget, "text")
self.view.controller.update_screen.assert_called_once_with()

def test_mouse_event(self, mocker, stream_view, widget_size):
def test_mouse_event(self, mocker, stream_view, mouse_scroll_event, widget_size):
event, button, key = mouse_scroll_event
mocker.patch.object(stream_view, "keypress")
size = widget_size(stream_view)
col = 1
row = 1
focus = "WIDGET"
# Left click
stream_view.mouse_event(size, "mouse press", 4, col, row, focus)
stream_view.keypress.assert_called_once_with(size, "up")

# Right click
stream_view.mouse_event(size, "mouse press", 5, col, row, focus)
stream_view.keypress.assert_called_with(size, "down")
stream_view.mouse_event(size, event, button, col, row, focus)
stream_view.keypress.assert_called_once_with(size, key)

@pytest.mark.parametrize("key", keys_for_command("SEARCH_STREAMS"))
def test_keypress_SEARCH_STREAMS(self, mocker, stream_view, key, widget_size):
Expand All @@ -589,7 +579,7 @@ def test_keypress_GO_BACK(self, mocker, stream_view, key, widget_size):
stream_view.log.clear()
stream_view.log.extend(stream_view.streams_btn_list[3])
stream_view.log.set_focus(0)
stream_view.keypress(size, "down")
stream_view.keypress(size, primary_key_for_command("GO_DOWN"))
assert stream_view.log.get_focus()[1] != stream_view.focus_index_before_search

# Exit search
Expand Down Expand Up @@ -718,7 +708,7 @@ def test_keypress_GO_BACK(self, mocker, topic_view, key, widget_size):
topic_view.log.clear()
topic_view.log.extend(topic_view.topics_btn_list[3])
topic_view.log.set_focus(0)
topic_view.keypress(size, "down")
topic_view.keypress(size, primary_key_for_command("GO_DOWN"))
assert topic_view.log.get_focus()[1] != topic_view.focus_index_before_search

# Exit search
Expand All @@ -730,6 +720,16 @@ def test_keypress_GO_BACK(self, mocker, topic_view, key, widget_size):
assert topic_view.log == topic_view.topics_btn_list
assert topic_view.log.get_focus()[1] == topic_view.focus_index_before_search

def test_mouse_event(self, mocker, topic_view, mouse_scroll_event, widget_size):
event, button, key = mouse_scroll_event
mocker.patch.object(topic_view, "keypress")
size = widget_size(topic_view)
col = 1
row = 1
focus = "WIDGET"
topic_view.mouse_event(size, event, button, col, row, focus)
topic_view.keypress.assert_called_once_with(size, key)


class TestUsersView:
@pytest.fixture
Expand All @@ -738,19 +738,15 @@ def user_view(self, mocker):
controller = mocker.Mock()
return UsersView(controller, "USER_BTN_LIST")

def test_mouse_event(self, mocker, user_view, widget_size):
def test_mouse_event(self, mocker, user_view, mouse_scroll_event, widget_size):
event, button, key = mouse_scroll_event
mocker.patch.object(user_view, "keypress")
size = widget_size(user_view)
col = 1
row = 1
focus = "WIDGET"
# Left click
user_view.mouse_event(size, "mouse press", 4, col, row, focus)
user_view.keypress.assert_called_with(size, "up")

# Right click
user_view.mouse_event(size, "mouse press", 5, col, row, focus)
user_view.keypress.assert_called_with(size, "down")
user_view.mouse_event(size, event, button, col, row, focus)
user_view.keypress.assert_called_with(size, key)

def test_mouse_event_left_click(
self, mocker, user_view, widget_size, compose_box_is_open
Expand Down Expand Up @@ -890,10 +886,9 @@ def test_keypress_SEARCH_MESSAGES(self, mid_col_view, mocker, key, widget_size):
)
mid_col_view.set_focus.assert_called_once_with("header")

@pytest.mark.parametrize("enter_key", keys_for_command("ENTER"))
@pytest.mark.parametrize("reply_message_key", keys_for_command("REPLY_MESSAGE"))
def test_keypress_REPLY_MESSAGE(
self, mid_col_view, mocker, widget_size, reply_message_key, enter_key
self, mid_col_view, mocker, widget_size, reply_message_key
):
size = widget_size(mid_col_view)
mocker.patch(MIDCOLVIEW + ".body")
Expand All @@ -903,7 +898,7 @@ def test_keypress_REPLY_MESSAGE(

mid_col_view.keypress(size, reply_message_key)

mid_col_view.body.keypress.assert_called_once_with(size, enter_key)
mid_col_view.body.keypress.assert_called_once_with(size, reply_message_key)
mid_col_view.set_focus.assert_called_once_with("footer")
assert mid_col_view.footer.focus_position == 1

Expand Down
4 changes: 2 additions & 2 deletions zulipterminal/ui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def keypress(self, size: urwid_Box, key: str) -> Optional[str]:
elif is_command_key("SEARCH_PEOPLE", key):
# Start User Search if not in editor_mode
self.body.focus_position = 2
self.users_view.keypress(size, "w")
self.users_view.keypress(size, key)
self.show_left_panel(visible=False)
self.show_right_panel(visible=True)
self.user_search.set_edit_text("")
Expand All @@ -233,7 +233,7 @@ def keypress(self, size: urwid_Box, key: str) -> Optional[str]:
):
# jump stream search
self.body.focus_position = 0
self.left_panel.keypress(size, "q")
self.left_panel.keypress(size, key)
self.show_right_panel(visible=False)
self.show_left_panel(visible=True)
if self.left_panel.is_in_topic_view:
Expand Down
12 changes: 6 additions & 6 deletions zulipterminal/ui_tools/boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
self.msg_write_box.edit_text = ""
if self.msg_edit_state is not None:
self.msg_edit_state = None
self.keypress(size, "esc")
self.keypress(size, primary_key_for_command("GO_BACK"))
elif is_command_key("GO_BACK", key):
self.msg_edit_state = None
self.msg_body_edit_enabled = True
Expand Down Expand Up @@ -1575,7 +1575,7 @@ def mouse_event(
return super().mouse_event(size, event, button, col, row, focus)

def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
if is_command_key("ENTER", key):
if is_command_key("REPLY_MESSAGE", key):
if self.message["type"] == "private":
self.model.controller.view.write_box.private_box_view(
emails=self.recipient_emails,
Expand Down Expand Up @@ -1653,15 +1653,15 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
recipient_user_ids=[self.message["sender_id"]],
)
elif is_command_key("MENTION_REPLY", key):
self.keypress(size, "enter")
self.keypress(size, primary_key_for_command("REPLY_MESSAGE"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes in this file likely work in practice, but mainly since we going through the intermediary of a str. At least, that's my perception of what's happening - can you see what I mean? We may want to make another change in this file to give these changes proper meaning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really sorry, but I don't get this. Could you please explain a bit more?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually was broken at this commit - there was no REPLY_MESSAGE handler for this and manual testing confirmed issues. Compare the difference between your local version and the version I pushed.

mention = f"@**{self.message['sender_full_name']}** "
self.model.controller.view.write_box.msg_write_box.set_edit_text(mention)
self.model.controller.view.write_box.msg_write_box.set_edit_pos(
len(mention)
)
self.model.controller.view.middle_column.set_focus("footer")
elif is_command_key("QUOTE_REPLY", key):
self.keypress(size, "enter")
self.keypress(size, primary_key_for_command("REPLY_MESSAGE"))

# To correctly quote a message that contains quote/code-blocks,
# we need to fence quoted message containing ``` with ````,
Expand Down Expand Up @@ -1723,7 +1723,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
msg_body_edit_enabled = False

if self.message["type"] == "private":
self.keypress(size, "enter")
self.keypress(size, primary_key_for_command("REPLY_MESSAGE"))
elif self.message["type"] == "stream":
self.model.controller.view.write_box.stream_box_edit_view(
stream_id=self.stream_id,
Expand Down Expand Up @@ -1846,7 +1846,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
self.panel_view.view.controller.exit_editor_mode()
self.reset_search_text()
self.panel_view.set_focus("body")
self.panel_view.keypress(size, "esc")
self.panel_view.keypress(size, primary_key_for_command("GO_BACK"))
elif is_command_key("ENTER", key) and not self.panel_view.empty_search:
self.panel_view.view.controller.exit_editor_mode()
self.set_caption([("filter_results", "Search Results"), " "])
Expand Down
35 changes: 18 additions & 17 deletions zulipterminal/ui_tools/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
KEY_BINDINGS,
is_command_key,
keys_for_command,
primary_key_for_command,
)
from zulipterminal.config.symbols import (
CHECK_MARK,
Expand Down Expand Up @@ -165,10 +166,10 @@ def mouse_event(
) -> bool:
if event == "mouse press":
if button == 4:
self.keypress(size, "up")
self.keypress(size, primary_key_for_command("GO_UP"))
return True
if button == 5:
self.keypress(size, "down")
self.keypress(size, primary_key_for_command("GO_DOWN"))
return True
return super().mouse_event(size, event, button, col, row, focus)

Expand Down Expand Up @@ -200,15 +201,15 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:

elif is_command_key("SCROLL_UP", key) and not self.old_loading:
if self.focus is not None and self.focus_position == 0:
return self.keypress(size, "up")
return self.keypress(size, primary_key_for_command("GO_UP"))
else:
return super().keypress(size, "page up")
return super().keypress(size, primary_key_for_command("SCROLL_UP"))

elif is_command_key("SCROLL_DOWN", key) and not self.old_loading:
if self.focus is not None and self.focus_position == len(self.log) - 1:
return self.keypress(size, "down")
return self.keypress(size, primary_key_for_command("GO_DOWN"))
else:
return super().keypress(size, "page down")
return super().keypress(size, primary_key_for_command("SCROLL_DOWN"))

elif is_command_key("THUMBS_UP", key):
if self.focus is not None:
Expand Down Expand Up @@ -365,10 +366,10 @@ def mouse_event(
) -> bool:
if event == "mouse press":
if button == 4:
self.keypress(size, "up")
self.keypress(size, primary_key_for_command("GO_UP"))
return True
elif button == 5:
self.keypress(size, "down")
self.keypress(size, primary_key_for_command("GO_DOWN"))
return True
return super().mouse_event(size, event, button, col, row, focus)

Expand Down Expand Up @@ -474,10 +475,10 @@ def mouse_event(
) -> bool:
if event == "mouse press":
if button == 4:
self.keypress(size, "up")
self.keypress(size, primary_key_for_command("GO_UP"))
return True
elif button == 5:
self.keypress(size, "down")
self.keypress(size, primary_key_for_command("GO_DOWN"))
return True
return super().mouse_event(size, event, button, col, row, focus)

Expand Down Expand Up @@ -514,11 +515,11 @@ def mouse_event(
return True
if button == 4:
for _ in range(5):
self.keypress(size, "up")
self.keypress(size, primary_key_for_command("GO_UP"))
return True
elif button == 5:
for _ in range(5):
self.keypress(size, "down")
self.keypress(size, primary_key_for_command("GO_DOWN"))
Comment on lines 517 to +522
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this refactor, but while mouse access is not a priority the 5x multiplier isn't consistent with the other boxes. I'd like this to be consistent, but equally I don't use a scroll-wheel mouse right now, so we can discuss in the stream.

I think this might have been brought up recently in discussion, but I'm not sure we reached a conclusion?

return super().mouse_event(size, event, button, col, row, focus)


Expand Down Expand Up @@ -574,8 +575,8 @@ def update_message_list_status_markers(self) -> None:

def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
if is_command_key("GO_BACK", key):
self.header.keypress(size, "esc")
self.footer.keypress(size, "esc")
self.header.keypress(size, key)
self.footer.keypress(size, key)
self.set_focus("body")

elif self.focus_position in ["footer", "header"]:
Expand All @@ -587,14 +588,14 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
return key

elif is_command_key("REPLY_MESSAGE", key):
self.body.keypress(size, "enter")
self.body.keypress(size, key)
Comment on lines 590 to +591
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is the start point you're referring to in the commit text?
So REPLY_MESSAGE here -> "enter" on box -> behavior like REPLY_MESSAGE via "ENTER" filter ?

This is a good find, but the commit text could be improved to make this clearer - what file/class source triggers what other ones?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this, but as noted I adjusted code between the last two commits to make this a little more accurate.

if self.footer.focus is not None:
self.set_focus("footer")
self.footer.focus_position = 1
return key

elif is_command_key("STREAM_MESSAGE", key):
self.body.keypress(size, "c")
self.body.keypress(size, key)
# For new streams with no previous conversation.
if self.footer.focus is None:
stream_id = self.model.stream_id
Expand All @@ -605,7 +606,7 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
return key

elif is_command_key("REPLY_AUTHOR", key):
self.body.keypress(size, "R")
self.body.keypress(size, key)
if self.footer.focus is not None:
self.set_focus("footer")
self.footer.focus_position = 1
Expand Down