Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 6, 2025

Followup to #78469.
We need to wait on https://devdiv.visualstudio.com/DevDiv/_git/VSEditor/pullrequest/634844 making it into a public preview to take this.

@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 6, 2025
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 6, 2025 21:24
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 6, 2025 21:24
this, textView, applicableToSpan, description,
cancelOnEdit, cancelOnFocusLost);

// Then add a single scope representing the how the UI should look initially.
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this 'AddScope' was pushed into the constructor.

// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
Copy link
Member Author

Choose a reason for hiding this comment

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

this file is effectively a rewrite. I would look at the SxS view, not hte inline view. And really just pay attention to how the new code is written.

@@ -1,89 +1,104 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
Copy link
Member Author

Choose a reason for hiding this comment

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

this file is effectively a rewrite. I would look at the SxS view, not hte inline view. And really just pay attention to how the new code is written.

// use the description of the last scope if we have one. We don't have enough room to show all
// the descriptions at once.
lock (this.ContextAndScopeDataMutationGate)
return _scopes_onlyAccessUnderLock.LastOrDefault()?.Description ?? _firstDescription;
Copy link
Contributor

Choose a reason for hiding this comment

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

_firstDescription

If the initial scope that was added during construction is removed, then it seems like we shouldn't be returning _firstDescription (which would allow removal of that member)

Copy link
Member Author

Choose a reason for hiding this comment

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

added comment. basically, there is always an implicit scope the platform holds onto which holds onto that first description. they just don't expose it. so this is how we simulate deferring to them.

public void TakeOwnership()
=> this.Dispose();

public IUIThreadOperationScope AddScope(bool allowCancellation, string description)
Copy link
Contributor

Choose a reason for hiding this comment

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

AddScope

Might be worth a comment why AddScope doesn't need to call ReportTotalProgress as it's not obvious to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. basically, the reason is simply that when a scope is created, it has no progress (it's effective 0 completed of 0 total). So it would have no impact on any progress display. happy to comment that.

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge May 7, 2025 07:00
@kayle
Copy link

kayle commented May 7, 2025

I don't think you need to change anything, but just wanted to clarify the platform can have multiple BWI indicators active. I don't think that helps any Roslyn scenarios I can think of, but it's available if helpful.

@CyrusNajmabadi
Copy link
Member Author

Understood. We've decided against that for our UX experiences :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants