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

Cancel outdated pending compose suggestions #6838

Merged

Conversation

ClearlyClaire
Copy link
Contributor

This PR cancels outdated compose suggestions requests to prevent outdated results to show up instead of newer (usually more precise) ones, or when the current composed text does not call for a completion.

@LienRag
Copy link

LienRag commented Mar 19, 2018

I don't get it, how do you decide what is obsolete or not?

@ClearlyClaire
Copy link
Contributor Author

@LienRag the most recently-issued request is the only one which results we are interested in. Any other request that might still be running can be canceled.

@ClearlyClaire
Copy link
Contributor Author

Hm, some unexpected behavior is still going on, and I'm not completely sure why: when typing something that could be a username (e.g. @gaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) then pressing backspace so it is quickly deleted, I seem to still get an answer for @ga. I guess the cancellation function might be created asynchronously and clearComposeSuggestions might be called too early?
Anyway, that's an edge case, the behavior is still way better for me.

Copy link
Member

@Gargron Gargron left a comment

Choose a reason for hiding this comment

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

This change is very welcome, but new js syntax is better

@@ -11,6 +12,9 @@ import {
refreshPublicTimeline,
} from './timelines';

var CancelToken = axios.CancelToken;
Copy link
Member

Choose a reason for hiding this comment

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

Use let

api(getState).get('/api/v1/accounts/search', {
cancelToken: new CancelToken(function (cancel) {
Copy link
Member

Choose a reason for hiding this comment

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

Use cancel => { for brevity

@ClearlyClaire ClearlyClaire force-pushed the fixes/cancel-compose-suggest-queries branch from 91ca509 to 7070f52 Compare March 19, 2018 22:22
@@ -11,6 +12,9 @@ import {
refreshPublicTimeline,
} from './timelines';

let CancelToken = axios.CancelToken;
let _CancelFetchComposeSuggestionsAccounts;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a sign that this is to remain private to this module. But this convention doesn't appear to be used widely/consistently within Mastodon, so I guess I can drop it (and also start with a lowercase).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm for dropping this because the code knows it, even without such a symbol. The code will not allow to use the variable out of this module.
If i recall correctly, underscore as prefix is seen in some components of Mastodon, but that could make sense because the code doesn't know a variable is private, and allows to use it out of the component (if ref is there.)

@ClearlyClaire ClearlyClaire force-pushed the fixes/cancel-compose-suggest-queries branch from 7070f52 to b5d1ebe Compare March 20, 2018 08:59
@@ -1,4 +1,5 @@
import api from '../api';
import axios from 'axios';
Copy link
Member

Choose a reason for hiding this comment

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

Oh I get it now

import axios, { CancelToken } from 'axios';

should work instead of let CancelToken

@ClearlyClaire ClearlyClaire force-pushed the fixes/cancel-compose-suggest-queries branch from b5d1ebe to 7546095 Compare March 20, 2018 11:07
@ClearlyClaire ClearlyClaire force-pushed the fixes/cancel-compose-suggest-queries branch from 7546095 to 08f49bd Compare March 20, 2018 11:12
@Gargron Gargron merged commit a5c6c74 into mastodon:master Mar 20, 2018
@ClearlyClaire ClearlyClaire deleted the fixes/cancel-compose-suggest-queries branch April 3, 2018 16:32
smorimoto pushed a commit to kibousoft/mastodon that referenced this pull request Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants