Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Add a way to stop an ongoing project search #7621

Merged
merged 6 commits into from
Jan 3, 2019
Merged

Add a way to stop an ongoing project search #7621

merged 6 commits into from
Jan 3, 2019

Conversation

Garee
Copy link
Contributor

@Garee Garee commented Dec 31, 2018

Summary

First, the search operation is assigned a cancellation function that can be used to stop the search. Next, when a new search is started, it is saved in state so that it can later be cancelled.

The ongoing search is cancelled when:

  • A new project search is started.
  • A user clicks the "x" button on the search input.

Fixes #7559

Testing

  • Verified that project searches work as before.
  • Performed searches in quick succession and observed that only one search was in progress at a time.
  • Tested cancellation via starting a new search and clicking on the "x" button.

Other

  • Do we want the "x" button to cancel the search, or should there be a new button?
  • The cancellation isn't immediate -- it occurs after the current source has been searched. This isn't perfect but it's a big improvement.

First, the search operation is assigned a cancellation function
that can be used to stop the search. Next, when a new search is
started, it is saved in state so that it can later be cancelled.

The ongoing search is cancelled when:

- A new project search is started.
- A user clicks the "x" button on the search input.
src/components/ProjectSearch.js Outdated Show resolved Hide resolved
src/components/ProjectSearch.js Outdated Show resolved Hide resolved
await dispatch(clearSearchResults());
await dispatch(addSearchQuery(query));
dispatch(updateSearchStatus(statusType.fetching));
const validSources = getSourceList(getState()).filter(
source => !hasPrettySource(getState(), source.id) && !isThirdParty(source)
);
for (const source of validSources) {
if (cancelled) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this 👍

src/actions/project-text-search.js Outdated Show resolved Hide resolved
src/reducers/project-text-search.js Outdated Show resolved Hide resolved
@AnshulMalik
Copy link
Contributor

This is great! @Garee :)

- Use ProjectTextSearch selectors
- Rename 'search' state to 'ongoingSearch'
- Move stopOngoingSearch() into searchSources() and closeProjectSearch()
src/reducers/project-text-search.js Outdated Show resolved Hide resolved
src/actions/project-text-search.js Outdated Show resolved Hide resolved
@AnshulMalik
Copy link
Contributor

Looks good to me, we'll probably need to set the status of the search as done or something when we cancel.
Should we stop the search if the user presses Esc key? @jasonLaster ?

@Garee
Copy link
Contributor Author

Garee commented Jan 1, 2019

Looks good to me, we'll probably need to set the status of the search as done or something when we cancel.

I added and set a new statusType.cancelled status.

Copy link
Contributor

@AnshulMalik AnshulMalik left a comment

Choose a reason for hiding this comment

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

LGTM :) Nice work!
We can do some follow up tweaks

@darkwing
Copy link
Contributor

darkwing commented Jan 2, 2019

@AnshulMalik What ideas do you have for follow up tweaks?

@AnshulMalik
Copy link
Contributor

@darkwing A few

  1. Clean up the ongoingSearch after we are done with the search
  2. May be use Esc key to stop the search in the middle.

@darkwing darkwing merged commit 494b0cf into firefox-devtools:master Jan 3, 2019
@darkwing
Copy link
Contributor

darkwing commented Jan 3, 2019

Great work @Garee ! I love it!

jasonLaster pushed a commit to jasonLaster/debugger.html that referenced this pull request Jan 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants