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.

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

Jump to bottom

Next / previous conversation keyboard shortcuts do not work in search results #5176

Open
hiqua opened this issue Apr 12, 2021 · 6 comments · May be fixed by #5234
Open

Next / previous conversation keyboard shortcuts do not work in search results #5176

hiqua opened this issue Apr 12, 2021 · 6 comments · May be fixed by #5234
Labels
Bug Good starter task

Comments

@hiqua
Copy link
Contributor

hiqua commented Apr 12, 2021

Bug (borderline feature) Description

Keyboard shortcuts to switch between conversations (Alt-down / Alt-up) do not work in search results.

Steps to Reproduce

  1. search for conversations
  2. obtain several results
  3. try to use alt-down to cycle through the results

Actual Result:

Nothing happens.

Expected Result:

Cycling through the conversations, just like in the main view.

Platform Info

Signal Version:

5.0.0-beta3

Operating System:

Debian
@EvanHahn-Signal
Copy link
Contributor

Thanks—I've filed this as a bug, and a good starter task.

This is happening because this method is unimplemented:

Signal-Desktop/ts/components/leftPane/LeftPaneSearchHelper.tsx

Lines 249 to 257 in 1c23efb

// This is currently unimplemented. See DESKTOP-1170.
// eslint-disable-next-line class-methods-use-this
getConversationAndMessageInDirection(
_toFind: Readonly<ToFindType>,
_selectedConversationId: undefined | string,
_selectedMessageId: unknown
): undefined | { conversationId: string } {
return undefined;
}

If you work on this, a few things to keep in mind:

  • Though it won't be the same, you can refer to how other left pane helpers do this. For example, you might want to follow the code in inbox helper.
  • The current return type for the function is undefined | { conversationId: string }. I suspect it will change to undefined | { conversationId: string; messageId?: string }.
  • Search results are in three groups: chats, contacts, and individual messages. Keep in mind that the currently selected row might not be in the same group if you move down. For example, if you've selected the final contact and press Alt-Down, the first message result will be selected.
  • Make sure to add tests in LeftPaneSearchHelper_test.ts. I expect there will be several new tests (probably more than 2 but fewer than 10) for this behavior.
  • Nothing should happen if any of the search results are loading.
  • Comment here if you're going to take it on, just to make sure nobody else wastes their time.
  • Let us know if you need help!

@jprask
Copy link

jprask commented Apr 28, 2021

Hey guys, may I give this a try?

@hiqua
Copy link
Contributor Author

hiqua commented Apr 28, 2021

@jprask sure!

@martineizayaga
Copy link

@jprask what's the status? Would you mind if I worked on this?

@jprask
Copy link

jprask commented May 5, 2021

Thanks for checking @martineizayaga I'm writing the tests for it right now.

jprask added a commit to jprask/Signal-Desktop that referenced this issue May 9, 2021
@jprask
Add getConversationAndMessageInDirection implementation to LeftPaneSe…
1bc33e2 
…archHelper

This allows user to navigate trough search results using the keyboard. Closes signalapp#5176.

The feature is more complex than one might initially think, due to the data structure for the messages being different of that from the conversations. This commit goes with the approach of figuring out if the next item in the navigation flow could be a message, then trying to find the next message and finally falling back to looking for the next conversation.

Another thing about the message data structure is that currently we can't check if it's unread, so in this commit when browsing trough unreads only the message results will be ignored.
@jprask jprask linked a pull request May 9, 2021 that will close this issue
Open
7 tasks
@hiqua hiqua mentioned this issue May 18, 2021
Open
@abdullahnauman2
Copy link

Hi! I'd love to work on this task. Would that be okay?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Good starter task
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow keyboard navigation in search results jprask/Signal-Desktop
5 participants
@hiqua @martineizayaga @jprask @EvanHahn-Signal @abdullahnauman2