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

Render reference results as they are returned by reference providers. #27886

Closed
wants to merge 12 commits into from
Closed

Conversation

nicksnyder
Copy link
Contributor

@nicksnyder nicksnyder commented Jun 2, 2017

This prevents a slow reference provider from blocking the results of other reference providers.

This PR is useful by itself, but it also resolves UX issues related to #20010 and gives us a starting point to implement streaming further down the stack.

Before
before

After (edit: updated screenshot with progress bar)
progress3

TODO

  • Fix no results case

Sorry, something went wrong.

@mention-bot
Copy link

@nicksnyder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jrieken and @egamma to be potential reviewers.

@jens1o
Copy link
Contributor

jens1o commented Jun 2, 2017

really cool.

@jrieken jrieken added editor-contrib Editor collection of extras feature-request Request for new features or functionality labels Jun 2, 2017
@jrieken jrieken added this to the June 2017 milestone Jun 2, 2017
@nicksnyder
Copy link
Contributor Author

nicksnyder commented Jun 5, 2017

I pushed a branch where you can play with this to see how the UI would behave for multiple concurrent reference providers (or a single streaming reference provider).

progress4

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
This prevents a slow reference provider from blocking the results of other reference providers.
return handleModel(result);
}
// All results should have been received via progress.
alert(this._model.getAriaMessage());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do this in a .then instead

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@nicksnyder
Copy link
Contributor Author

nicksnyder commented Jun 6, 2017

I added an indefinite loading indicator for streaming references and fixed tests. Animated gifs and streamdemo branch have been updated.

@nicksnyder
Copy link
Contributor Author

I merged in master to resolve a conflict but tests are failing on master. Whenever tests are fixed on master, can re-merge in master to fix tests for this PR.

@nicksnyder
Copy link
Contributor Author

build is passing now

@nicksnyder nicksnyder mentioned this pull request Jun 16, 2017
@jrieken jrieken modified the milestones: July 2017, June 2017 Jun 26, 2017
@jrieken
Copy link
Member

jrieken commented Jun 26, 2017

Moving to July. I have seen how this is used in source graph but it needs more work to make sure a stable UI is guaranteed.

@nicksnyder
Copy link
Contributor Author

it needs more work to make sure a stable UI is guaranteed.

I am happy to work on this more if you care to share what concerns still remain.

return undefined;
}, err => {
onUnexpectedExternalError(err);
export function provideReferences(model: editorCommon.IReadOnlyModel, position: Position): PPromise<void, Location[]> {
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of PPromise. I'd suggest to leave it TPromise<Location[]> that resolves with all locations but have an optional argument IProgess<T>, as it's defined here. Something like this

export function provideReferences(
  model: editorCommon.IReadOnlyModel, 
  position: Position, 
  progress?: IProgress<Location[]>
): TPromise<Location[]> {

  // ...
}

The looks are nice but it's also closer to how this would be done with native promises. Also, less changes in unrelated files are required with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toggleWidget requires that I pass in some object that contains the callback. The options I considered are

  1. Use PPromise
  2. Create a new object that merely holds the callback function
  3. Refactor toggleWidget

I initially implemented (2) but tried (1) and thought it was cleaner. You would prefer (2)?

This is a situation where it would really be great to just have an observable type object: #28851

}

public setReferences(references: Location[]) {
this._setReferences(references, true);
Copy link
Member

@jrieken jrieken Jul 17, 2017

Choose a reason for hiding this comment

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

I think this should be an append-style method, so instead of resetting and resorting all locations we should always have append-insert semantics. So, if provider A returned A and C and if slow provider B returns B the order should be A, C, B. My goal is a stable UI, think of the following: A and C are in the UI and the user is just hovering over C to select it. At that time B is getting resolved. It should not push down C because the user would then select B instead of C. The tricky bit is that we are talking about a tree. I not sure what to do when the tree contains A/B and C/D, the user is selecting D and at the same time another child is inserted into the A-subtree. It will push down the C-subtree, making the user click the wrong item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the concern that you have, but I disagree on some points.

It should not push down C because the user would then select B instead of C.

You are of course theoretically correct. My arguments here are practical:

  1. It doesn't happen that often in practice
  2. Clicking on the wrong thing doesn't cost the user anything (UI doesn't change in a dramatic way, can just click again on the intended result)
  3. We could throttle updates to predictable intervals (e.g every 1s) so the user can intuitively know when an update might happen.
  4. The user can just wait for all results to return (which is what they are forced to do now) before interacting. They are strictly better off with streaming.
  5. As long as the user's current selection/view is maintained, I think it is ok for the results around it to be updating.

It will push down the C-subtree, making the user click the wrong item.

Only if subtree A is expanded. I forget what the behavior is in the PR as it stands now, but we wouldn't need to auto expand any subtree automatically until all results are in. I think the auto expand behavior depends on # results in a subtree so it wouldn't make sense to auto expand anyway until all results are in.

instead of resetting and resorting all locations we should always have append-insert semantics.

I got the opposite feedback on #28751, because the sorting was relevant there. As of right now, I would agree that the sorting isn't as relevant for references here (still a non-zero amount of relevant), but we would be precluding it from being more relevant in the future.

It also seems a little bit inconsistent if we have different behaviors for streaming in different contexts.


I am not sure if I will have time to make progress on this in the next two weeks.

@jrieken jrieken modified the milestones: July 2017, August 2017 Jul 21, 2017
@jrieken jrieken modified the milestone: August 2017 Aug 17, 2017
@nicksnyder nicksnyder closed this Oct 5, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-contrib Editor collection of extras feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants