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

Implement new interfaces for adding files #30527

Merged
merged 1 commit into from
Nov 9, 2018
Merged

Implement new interfaces for adding files #30527

merged 1 commit into from
Nov 9, 2018

Conversation

tmeschter
Copy link
Contributor

Implement a new set of interfaces to allow csproj/msvbprj to add files.
The existing ICSharpProjecSite.OnSourceFileAdded and
IVbCompilerProject.AddFile had to go back to the hierarchy to check if
the file had any <Link> metadata on it, effectively requiring that
these methods only be called on the main thread. However, this metadata
is readily available to csproj/msvbprj; the new
OnSourceFileEx`AddFileEx` methods allow it to be passed in. This
should make it possible to call these methods on a background thread.

@tmeschter tmeschter requested a review from a team as a code owner October 15, 2018 23:00
@tmeschter
Copy link
Contributor Author

Note I do not actually intend to merge this into dev/jasonmal/free-threaded-project-system-api-vs-deps but as that branch hasn't merged yet, this shows what has actually changed.

@jasonmalinowski jasonmalinowski force-pushed the dev/jasonmal/free-threaded-project-system-api-vs-deps branch from 41546c6 to d19fd35 Compare October 16, 2018 23:56
@jasonmalinowski jasonmalinowski changed the base branch from dev/jasonmal/free-threaded-project-system-api-vs-deps to dev16.0.x October 19, 2018 05:30
@jasonmalinowski
Copy link
Member

@tmeschter I've pointed this PR now at the dev16.0.x branch but a word of explanation: the branch you were targeting was me merging my branch (which went into dev16.0.x) with dev16.0.x-vs-deps to make sure I had any changes there that might impact RPS. As a result, your branch implicitly is including those. You're going to have to do a git rebase <remote>/dev16.0.x-vs-deps --onto <remote>/dev16.0.x and repush.

@tmeschter
Copy link
Contributor Author

@jasonmalinowski Yes, I was originally targeting your branch in order to make it easier to review--the my plan was always to wait until your change was in and then rebase my change.

@tmeschter tmeschter requested review from a team as code owners October 23, 2018 21:32
@tmeschter tmeschter changed the base branch from dev16.0.x to master October 23, 2018 21:33
@tmeschter
Copy link
Contributor Author

@jasonmalinowski I've rebased the changes and updated the PR to point to master.

jasonmalinowski
jasonmalinowski approved these changes Oct 23, 2018
View reviewed changes
src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpProjectShim.ICSharpProjectSiteEx.cs Outdated
{
internal partial class CSharpProjectShim : ICSharpProjectSiteEx
{
public void OnSourceFileAddedEx(string filename, string folder)
Copy link
Member

Choose a reason for hiding this comment

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

Is there an agreement here that synchronization is being done by the project system to avoid parallel calls?

Copy link
Member

Choose a reason for hiding this comment

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

(As it stands, your AddFile isn't mutating any state and VisualStudioProject is fine itself...but I'm not sure if that'll ever hold if somebody else adds something further to AddFile.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's on the project system to deal with that.

Ideally these interfaces are just a temporary stepping stone and I'll eventually be able to remove them in favor of the legacy project system shims implementing IWorkspaceProjectContext.

@tmeschter
Copy link
Contributor Author

test this

@tmeschter
Copy link
Contributor Author

@dotnet-bot test this

@tmeschter
Copy link
Contributor Author

test this please

@tmeschter tmeschter closed this Oct 24, 2018
@tmeschter tmeschter reopened this Oct 24, 2018
@tmeschter
Copy link
Contributor Author

test this please

@jinujoseph jinujoseph added this to the 16.0.P2 milestone Oct 24, 2018
@tmeschter
Implement new interfaces for adding files
5b250f6 
Implement a new interface to allow csproj/msvbprj to add files.
The existing `ICSharpProjecSite.OnSourceFileAdded` and
`IVbCompilerProject.AddFile` had to go back to the hierarchy to check if
the file had any `<Link>` metadata on it, effectively requiring that
these methods only be called on the main thread. However, this metadata
is readily available to csproj/msvbprj; the new
`IProjectSiteEx.AddFilEx` method allow it to be passed in. This
should make it possible to call this method on a background thread.

Also, the language service supports batching up project changes before
pushing them to the workspace, but does not currently expose a way for
csproj/msvbprj to tell it when a batch of changes is starting or ending.
The `StartBatch` and `EndBatch` methods on `IProjectSiteEx` fill this
need.
@genlu
Copy link
Member

genlu commented Oct 29, 2018

@tmeschter A somewhat related question: are we gonna have a flag or something on the document to indicate it's linked (i.e. using ) vs. just a regular document (e.g. could be a document in a MTFM project, which has multiple documents in the solution with same path, but not necessarily a linked document)

@tmeschter
Copy link
Contributor Author

@genlu I don't think I fully understand your question, but this work doesn't change anything about how source files (with <Link> metadata or not) are represented within the Roslyn workspace.

@genlu
Copy link
Member

genlu commented Oct 29, 2018

I'm just curious, because the information that if and how a document is linked into the project is not readily available in workspace. There's GetLinkedDocumentIds, but it can't differentiate between , shared project and MTFM project, etc.

@tmeschter
Copy link
Contributor Author

test windows_release_vs-integration_prtest

@tmeschter tmeschter changed the base branch from master to dev16.0-preview2 November 6, 2018 21:01
@tmeschter
Copy link
Contributor Author

test this please

@tmeschter
Copy link
Contributor Author

@Pilchie I just retargeted this to dev16.0-preview2; do I still have approval to merge it in?

@jinujoseph
Copy link
Contributor

@tmeschter yes you need M2 Approval for preview2

@agocke is the branch ready for preview2 changes ?

Pilchie
Pilchie approved these changes Nov 7, 2018
View reviewed changes
src/VisualStudio/Core/Def/Implementation/ProjectSystem/Legacy/AbstractLegacyProject.cs
var folders = ImmutableArray<string>.Empty;
if (!string.IsNullOrEmpty(linkMetadata))
{
var linkFolderPath = Path.GetDirectoryName(linkMetadata);
Copy link
Member

Choose a reason for hiding this comment

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

Has something validated that this is valid filename? If not, I believe this will throw

@tmeschter
Copy link
Contributor Author

test windows_release_vs-integration_prtest

@tmeschter
Copy link
Contributor Author

test integration

@tmeschter tmeschter closed this Nov 8, 2018
@tmeschter tmeschter reopened this Nov 8, 2018
@tmeschter tmeschter merged commit 7f47498 into dotnet:dev16.0-preview2 Nov 9, 2018
@tmeschter tmeschter deleted the NewAddFileApis-181015 branch November 9, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@Pilchie Pilchie Pilchie approved these changes

@heejaechang heejaechang heejaechang approved these changes

Assignees
No one assigned
Labels
Approved to merge Area-IDE
Projects
None yet
Milestone
16.0.P2
Development

Successfully merging this pull request may close these issues.
6 participants
@tmeschter @jasonmalinowski @genlu @jinujoseph @Pilchie @heejaechang