Skip to content
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

bugfix: view: Fix Zulip crash during search. #977

Closed

Conversation

Rohitth007
Copy link
Collaborator

@Rohitth007 Rohitth007 commented Apr 2, 2021

This commit fixes a bug which causes Zulip to crash when searching
Streams or Topics. This happens because when a search result is
empty, log is empty and because an empty ListWalker returns None,
it leads to a Type error when setting focus.

Commit 1: Fixes update errors and unread count edge case.

  • This is caused when ZT recieves an update event while searching and
    set_count assumes that all widgets in the log are Buttons since
    the log is updated while searching.
  • This also means that message updates are only sent to a subset of
    streams, topics or users. Which also leads to missed updates and
    negative counts.
  • Since, we only need to update the count variable in the button
    object this is solved by using btn_list instead of log.

Solves part of #642

Commit 2: Fix Zulip crash during search.

  • This commit fixes a bug which causes Zulip to crash when searching
    Streams or Topics. This happens because when a search result is
    empty, log is empty and because an empty ListWalker returns None,
    it leads to a Type error when setting focus.
  • This is fixed by making sure the user does not enter the empty list
    when the search result is empty. The boolean empty_search is used
    to achieve this.
  • This commit also improves the UI/UX when search result is empty by
    displaying Text Not Found in place of the empty list.
    The user can then correct their search and Not Found disappears.
  • A search_error attribute is introducted in the pallete which
    colors Not Found in light red. The cross_mark symbol -
    STREAM_MARKER_INVALID is also added for effect.

Fixes #923

Commit 3: Name change for cross symbol for consistency.

  • STREAM_MARKER_INVALID has been changed to INVALID_MARKER so
    that the cross symbol can be used in multiple places not necessarily
    related to streams. Eg: Search error indicator.

Commit 4: Change name of topics_to_display for consistency.

  • The name of the variable topics_to_display has been changed to
    topic_display to maintain consistency with stream_display
    and user_display.
  • This can be dropped it it feels unnecessary.

Commit 5: Test split for better readability.

  • The tests for PanelSearchBox introduced when fixing bug crash while using search #923 is
    split into two tests for better readability.
  • Both cases test_keypress_ENTER_focus_set and
    test_keypress_ENTER_focus_not_set have to be checked carefully to
    avoid the bug.

Commit 6: Erase caption when reattempting search.

  • set_content('') is used so that when a user presses q for a
    second search the caption becomes empty.
  • This makes for a cleaner UI while searching by removing caption
    Search Results when typing.

@zulipbot zulipbot added size: M [Automatic label added by zulipbot] bug: crash labels Apr 2, 2021
@Rohitth007 Rohitth007 changed the title bugfix: view: Fix Zulip crash during search. [WIP] bugfix: view: Fix Zulip crash during search. Apr 2, 2021
@Rohitth007 Rohitth007 force-pushed the bugfix-923-and-ui-improvement branch from 6aa7140 to 024e380 Compare April 2, 2021 11:48
@Rohitth007
Copy link
Collaborator Author

@zulipbot add "PR needs review"

@zulipbot zulipbot added the PR needs review PR requires feedback to proceed label Apr 2, 2021
@Rohitth007 Rohitth007 changed the title [WIP] bugfix: view: Fix Zulip crash during search. bugfix: view: Fix Zulip crash during search. Apr 2, 2021
@Rohitth007
Copy link
Collaborator Author

@neiljp Does this need any more changes?

@Rohitth007 Rohitth007 force-pushed the bugfix-923-and-ui-improvement branch from 024e380 to 871e49c Compare April 6, 2021 18:31
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Apr 6, 2021
@Rohitth007 Rohitth007 force-pushed the bugfix-923-and-ui-improvement branch from 871e49c to d8b789d Compare April 7, 2021 12:48
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Rohitth007 This has great potential 👍 There are quite a few moving parts here, so I'd like to see them split into independent parts where they are independent, to make it clearer what each part does. The search reset seems like a separate matter entirely to the changes necessary for the UI change which resolves the bug?

tests/ui/test_ui_tools.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Show resolved Hide resolved
zulipterminal/ui_tools/views.py Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
tests/ui_tools/test_boxes.py Outdated Show resolved Hide resolved
zulipterminal/helper.py Show resolved Hide resolved
zulipterminal/ui_tools/views.py Show resolved Hide resolved
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Apr 9, 2021
@Rohitth007 Rohitth007 force-pushed the bugfix-923-and-ui-improvement branch from d8b789d to 0731e14 Compare April 12, 2021 11:09
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 12, 2021
@Rohitth007
Copy link
Collaborator Author

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"

@zulipbot zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 12, 2021
@Rohitth007
Copy link
Collaborator Author

@neiljp I have split the PR as suggested, I would like to know your views on some of the pending conversations so that they can be marked resolved.

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Apr 17, 2021
@neiljp neiljp added this to the Next Release milestone Apr 17, 2021
@Rohitth007 Rohitth007 force-pushed the bugfix-923-and-ui-improvement branch from 0731e14 to ed4e13b Compare April 18, 2021 08:52
@Rohitth007
Copy link
Collaborator Author

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"
@neiljp I have made the suggested changes and also moved topic_display name change into a separate commit. If this feels unnecessary it can be dropped while merging.

@zulipbot zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Apr 18, 2021
This is caused when ZT recieves an update event while searching and
`set_count` assumes that all widgets in the log are Buttons since
the log is updated while searching.

This also means that message updates are only sent to a subset of
streams, topics or users. Which also leads to missed updates and
negative counts.

Since, we only need to update the count variable in the button
object this is solved by using `btn_list` instead of `log`.

Solves part of zulip#642
This commit fixes a bug which causes Zulip to crash when searching
Streams or Topics. This happens because when a search result is
empty, log is empty and because an empty ListWalker returns None,
it leads to a Type error when setting focus.

This is fixed by making sure the user does not enter the empty list
when the search result is empty. The boolean `empty_search` is used
to achieve this.

This commit also improves the UI/UX when search result is empty by
displaying Text `Not Found` in place of the empty list.
The user can then correct their search and `Not Found` disappears.

A `search_error` attribute is introducted in the pallete which
colors `Not Found` in `light red`. The `cross_mark` symbol -
`STREAM_MARKER_INVALID` is also added for effect.

Fixes zulip#923

Tests amended.
`STREAM_MARKER_INVALID` has been changed to `INVALID_MARKER` so
that the cross symbol can be used in multiple places not necessarily
related to streams. Eg: Search error indicator.
The name of the variable `topics_to_display` has been changed to
`topic_display` to maintain consistency with `stream_display`
and `user_display`.
The tests for `PanelSearchBox` introduced when fixing bug zulip#923 is
split into two tests for better readability.

Both cases `test_keypress_ENTER_focus_set` and
`test_keypress_ENTER_focus_not_set` have to be checked carefully to
avoid the bug.
`set_content('')` is used so that when a user presses `q` for a
second search the caption becomes empty.

This makes for a cleaner UI while searching by removing caption
`Search Results` when typing.

Tests amended.
@Rohitth007 Rohitth007 force-pushed the bugfix-923-and-ui-improvement branch from b19d913 to c45253c Compare April 21, 2021 16:16
@neiljp
Copy link
Collaborator

neiljp commented May 17, 2021

@Rohitth007 Thanks for working on this! The bugfixes are great and the no-results UI is a definite improvement 👍 As per messages in the stream I've merged the unread counts bugfix and the crash bugfix/feature (plus symbol rename), with slight changes 🎉

I've left the other commits:

  • the test split ended up with a lot of duplication, and the conditional looks OK to leave as-is in comparison
  • the name-change isn't too important; I'd be more interested in seeing deduplication of these panel implementations in general, a little like we have for PanelSearchBox (the user-list case would need refactoring first though)
  • the search improvement with the caption reset is probably part of what we want - I've brought it up in the stream under Panel search box UI

On that basis I'm going to close this now, and I'd suggest we build on that last commit in a new PR once we settle on a UI.

@neiljp neiljp closed this May 17, 2021
@Rohitth007
Copy link
Collaborator Author

Thanks for merging this 🎉 . Having a common Panel would be brilliant, it would be easier to maintain and create new ones, but care should be taken for the user list as you said as there is no the UserView and RightColumn are mixed up. I'll look into this 👍

@Rohitth007 Rohitth007 deleted the bugfix-923-and-ui-improvement branch June 5, 2021 14:14
@Rohitth007 Rohitth007 restored the bugfix-923-and-ui-improvement branch June 5, 2021 14:14
@neiljp neiljp added the enhancement New feature or request label Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: crash enhancement New feature or request PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash while using search
3 participants