[WIP] User list section/filters #1248
Conversation
|
@srdeotarse We discussed this in the stream, so I'm marking it as awaiting update on the assumption that we've clarified how to move forward with this. Let me know if you need more feedback/review. |
neiljp
added
PR awaiting update
Split the function into two functions - 1) build_user_view - prepares the Users button list based on users input and returns the user button list. 2) users_view - calls build_user_view to use user button list to prepare the UsersView. This refactor will help for adding Recipients list in addition to current Users list and have toggling feature between then similar to streams and topics view.
Changed RightColumnView to urwid.Pile and UsersView to urwid.Frame similar to LeftColumnView and StreamsView/TopicsView. Shifted update_user_list and build_user_view functions from RightColumnView to UsersView. Change view.users_view to view.user_w in _start_presence_updates function in model.
Shifted from rigth_column_view in ui.py to users_view in RightColumnView in views.py.
Based on current narrow, the function return recipients list. All_messages and streams narrow is handled. PMs_all, PMs(1 to 1) and Groups narrow remaining.
Also added update_recipients_view, show_recipients_view and show_user_view functions.
srdeotarse
force-pushed
the
issue-575-user-section-filtering
branch
from
b6b646b
to
fa953b8
Compare
srdeotarse
added
PR needs review
There was a problem hiding this comment.
@srdeotarse Thanks for looking at this! This is so large, and promises to be more commits, since it's really a big tidy/refactor first, followed by the feature itself :)
I seem to have written an essay of notes to help with the preliminary refactor ;) Whenever adjusting working code, it's useful to break it into almost too many independent working commits, as it tends to make each commit smaller and easier to read, and so take less energy and time to review. git can tell me if code has just been moved in a commit, but when it's interspersed with other changes then it's more difficult to validate.
While you could adapt tests for your giant code move in one go, it would likely be a lot of changes there too, which are also then difficult to associate with a given code change without breaking into smaller commits.
I almost suggested adding tests for the new model method, but like I said there, I'm not sure if you want to use it to provide the appropriate list/set of user ids only instead. That may be more obvious as you make the PR slightly more functional.
While you hopefully make sense of the refactoring splitting, for the functionality of the recipients part, there are various elements to address:
- toggling-back from recipients (we discussed); currently the list is only updated to the filtered view on toggling, so it would be useful to know it can be explored/tested manually by toggling back and forth in different narrows :)
- updating reliably from regular refreshes; this currently does an all-users view, since the user list assumes on updating, that the user list is the list to display. Maybe handle the filtering in the UsersView?
- updating on changing narrow; this is a similar problem to the 'highlight button representing active narrow' PR
| def users_view(self, users: Any = None) -> Any: | ||
| users_btn_list = self.build_user_view(users) | ||
| user_w = UsersView(self.view.controller, users_btn_list) | ||
| # Donot reset them while searching. | ||
| if self.reset_default_view_users: | ||
| self.users_btn_list = users_btn_list | ||
| self.view.user_w = user_w |
There was a problem hiding this comment.
This function looks quite different, with quite significant other changes, other than just for the move?
| self.users_btn_list = users_btn_list | ||
| self.log = urwid.SimpleFocusListWalker(users_btn_list) |
There was a problem hiding this comment.
This is a material change, not a refactor.
We don't appear to currently modify the log so we may not strictly need it, but it does not need removing here.
| user_w = UsersView(self.view.controller, users_btn_list) | ||
| # Donot reset them while searching. | ||
| if reset_default_view_users: | ||
| if self.reset_default_view_users: | ||
| self.users_btn_list = users_btn_list | ||
| self.view.user_w = user_w | ||
| return user_w |
There was a problem hiding this comment.
This was just going to be a comment on this change, but it turned into a review of the refactoring as one comment.
I think you've adapted this change fairly straight from a commit in #459 ?
The difference appears to be you've tried to keep users_view in RightColumnView, which I only just noticed, but shows up significantly in the move commit. It's fine to take this approach and may even make it clearer what's happening, once we simplify the refactoring.
One issue that jumps out at me is how here you rely upon the variable that is now a new member variable, but only used between these two functions - which I think was in the original commit, but that doesn't necessarily make it ideal :)
This commit functionally works, but you later move the build function to UsersView, without this function. That's part of the movement of code, but you drop the later conditional entirely at that point with no explanation. It also leaves the member variable being set, but then it doesn't get used. There's no guarantee that we break a feature by doing all of that, but it's the kind of thing I'd expect to see in a separate commit to remove it cleanly with reasons, if it was intentional.
More generally, if we want an end goal of a users_view in RightColumnView having very little code in other than setting up UsersView, and generally moving/changing as little code as possible in the moving-code refactor step(s) (like the next one) to make it clear that code has been moved and not subtly changed, then this commit needs to change and likely become multiple commits to prepare for the move. If I look at the diff of the next commit and what changes other than moving, this includes:
- users_view (staying in RightColumnView) needs pre-adjusting to have
- no parameters
- no build_user_view (it moves away)
- to continue to handle the conditional in this function
- consider what the original check does, depends on, and where it could go instead
- UsersView.init
- change from controller to view (this was a previous separate [WIP] - Make UsersView consistent with other Views (Streams/Topic) #459 commit)
- users_btn_list default value? a required named value is clearer using
,*,nameand you could rename it at the same time if that made it clearer
- UsersView parent class change?
- I'm not sure if this is too connected with the code move to separate, but it might be a small thing to split out? I'd need to try that to confirm
- build_user_view
- depending how you restructure this given the above, you may be able to move it separately to the rest? (first?)
- I'd suggest an underscore prefix since it's used internally, and perhaps a more appropriate name like button_list_from_users?
Generally the diff will also be clearer to read if adjacent code that's moved doesn't get split, which is noticeable with the diff colorMoved option.
| view.users_view.update_user_list(user_list=self.users) | ||
| view.user_w.update_user_list(user_list=self.users) |
There was a problem hiding this comment.
I think we have similar names to the new one in the view, but we already have view.users_view too. This is duplicative and confusing to readers of the code, or this commit.
Apparently currently view.users_view is actually a RightColumnView, and seemingly ends up being a duplicate of right_panel. So another good refactor before this move commit, would be to remove view.users_view and adapt for its removal.
| self.user_v = self.users_view() | ||
| contents = [self.user_v] | ||
| super().__init__(contents) |
There was a problem hiding this comment.
Are you using self.user_v anywhere else?
I'd suggest at least combining the last two lines.
| @@ -265,7 +252,7 @@ def keypress(self, size: urwid_Box, key: str) -> Optional[str]: | |||
| self.show_left_panel(visible=False) | |||
| self.show_right_panel(visible=True) | |||
| self.body.focus_position = 2 | |||
| self.users_view.keypress(size, key) | |||
| self.right_panel.keypress(size, key) | |||
There was a problem hiding this comment.
This was the kind of change I was referring to with the users_view vs users_w change, which could go into a separate commit to this, before the move commit(s).
| def get_recipients_of_current_narrow( | ||
| self, current_narrow: List[Any] |
There was a problem hiding this comment.
Either make this the current narrow, or one passed in; it's confusing to say current and then pass one in. The model tracks this already.
| subscribers_list = self.get_other_subscribers_in_stream( | ||
| stream_name=current_narrow[0][1] | ||
| ) | ||
| recipients_list = [ | ||
| recipient | ||
| for recipient in self.get_all_users() | ||
| for recipient_id in subscribers_list | ||
| if recipient["user_id"] == recipient_id | ||
| ] |
There was a problem hiding this comment.
This seems to be a nested loop over get_all_users data - which has the right data for output - with a nested loop over the limited subscriber ids to filter them out?
It would be clearer to read, and faster, if you used membership testing for the last part.
I'm guessing this filtering part will be common to all narrows, and we might even want it external to this function?
| def update_recipients_view(self) -> None: | ||
| self.user_v = self.recipients_view() | ||
| if not self.is_in_recipients_view: | ||
| self.show_user_view() | ||
|
|
||
| def show_user_view(self) -> None: | ||
| self.is_in_recipients_view = False | ||
| self.contents[0] = (self.user_v, self.options(height_type="weight")) | ||
|
|
||
| def show_recipients_view(self) -> None: | ||
| self.is_in_recipients_view = True | ||
| self.contents[0] = (self.recipients_view(), self.options(height_type="weight")) |
There was a problem hiding this comment.
What does self.user_v represent? I asked why you had it earlier, and you didn't need to have it be a member then I think. You could add it here and give it a more meaningful name.
I'm confused over your use of it in update_recipients_view and show_user_view, though you don't seem to be using the former.
| if is_command_key("TOGGLE_RECIPIENTS_VIEW", key): | ||
| self._view.right_panel.show_recipients_view() |
There was a problem hiding this comment.
This seems kinda OK here, though it feels like it should go higher up, eg. RightColumnView?
neiljp
added
PR awaiting update
|
Heads up @srdeotarse, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
What does this PR do?
This WIP PR migrates earlier struture of
RightColumnViewinviews.pyto structure similar toLeftColumnViewwith toggling betweenStreamsViewandTopicsView.This PR mainly intends to add new
RecipientsViewthat displays recipients of the current narrow(All_messages,PMs,Streams,Topicsetc) in the right column.This fixes #575
CZO - https://chat.zulip.org/#narrow/stream/206-zulip-terminal/topic/User.20list.20sections.2Ffilters.20.23T575
Tested?
Commit flow
Notes & Questions
This WIP PR only includes transition from earlier structure of
RightColumnViewto new structure based onLeftColumnViewstructure. It includes modifications toUserViewInteractions
This PR is motivated by PR #459
Visual changes