Don't trigger rename of code element if filename has not been changed. #920

jinujoseph · 2016-12-12T20:38:46Z

fixes #363 , fixes #1063
@davkean @srivatsn @dotnet/project-system

333fred

Can we add a test for this?

333fred · 2016-12-12T20:50:27Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/FileRenameTracker.cs

@@ -68,6 +68,11 @@ public Task HintingAsync(IProjectChangeHint hint)

        private void ScheduleRename(string oldFilePath, string newFilePath)
        {
+            if (Equals(Path.GetFileName(oldFilePath),Path.GetFileName(newFilePath)))


formatting nit: spacing between arguments.

jinujoseph · 2016-12-12T20:51:06Z

will add tests

davkean · 2016-12-13T02:53:35Z

...rosoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Rename/SimpleRenameStrategy.cs

@@ -23,7 +23,7 @@ public override bool CanHandleRename(string oldFileName, string newFileName)
        {
            var oldNameBase = Path.GetFileNameWithoutExtension(oldFileName);
            var newNameBase = Path.GetFileNameWithoutExtension(newFileName);
-            return SyntaxFacts.IsValidIdentifier(oldNameBase) && SyntaxFacts.IsValidIdentifier(newNameBase);
+            return SyntaxFacts.IsValidIdentifier(oldNameBase) && SyntaxFacts.IsValidIdentifier(newNameBase) && !Equals(Path.GetFileName(oldNameBase), Path.GetFileName(newNameBase));
        }


This is object.Equals, you should be using StringComparers.Path.

Oh, it's by design that takes into account case? Unsure what I think about this - given most Windows file systems are case insensitive

And this probably just represents the case of the <Compile Include="foo.cs" /> in the project file - and should this result in a rename for VB which is also case-insenstive?

@davkean , i am comparing the filename's not path . Have switched to using string.Compare.
I compared this with VS 14 and the rename is case sensitive. So rename is triggered if you change from Foo to foo. Let me know if you think otherwise

davkean · 2016-12-15T04:21:05Z

Spent some time about this, and I think this is what we should:

For C#, case should be factored in and we should rename program -> Program and vice versa.
For VB, however, I don't think case should be factored in.

This matches the legacy project system's behavior.

We should try and do above in a language-agnostic way - ie by asking some service from Roslyn based on the document and see if that can tell us whether it's case or a case-insensitive language to avoid hard-coding a language.

davkean · 2016-12-15T06:46:53Z

Okay, did some more research, basically, what you want to do is look at the doc and do something like:

this.Document.GetLanguageService<ISyntaxFactsService>().IsCaseSensitive;

jinujoseph · 2016-12-15T14:28:14Z

@davkean thanks for taking a look, will add the conditional check

davkean · 2017-01-03T03:49:21Z

@jinujoseph Can you see if this resolves #1063? It looks like we're kicking off a bunch of rename sessions when dragging a bunch of files around.

jinujoseph · 2017-01-03T22:57:35Z

@davkean: My understanding from @mavasani is that in order to use ISyntaxFactsService I need to have IVT access to Roslyn workspaces, so i will have to add IVT access, publish new nugetpackages for roslyn and add corresponding rolsyn dependencies to workspaces. Manish suggested i use compilation object instead [http://source.roslyn.io/#Microsoft.CodeAnalysis/Compilation/Compilation.cs,a12821839905319d,references ] , Let me know if you forsee any issue here.

I will validate 1063 on big project.

mavasani · 2017-01-03T23:34:22Z

Note that Compilation.IsCaseSensitive will only work for projects supporting Compilation, i.e. VB and C# for now, which is probably fine until we support another language such as TypeScript which doesn't support compilation.

davkean · 2017-01-04T00:18:55Z

Sounds fine.

jinujoseph · 2017-01-04T19:08:13Z

verified that this resolves #1063

mavasani · 2017-01-04T19:10:56Z

...rosoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Rename/SimpleRenameStrategy.cs

        {
            var oldNameBase = Path.GetFileNameWithoutExtension(oldFileName);
            var newNameBase = Path.GetFileNameWithoutExtension(newFileName);
-            return SyntaxFacts.IsValidIdentifier(oldNameBase) && SyntaxFacts.IsValidIdentifier(newNameBase);
+            return SyntaxFacts.IsValidIdentifier(oldNameBase) && SyntaxFacts.IsValidIdentifier(newNameBase) && (string.Compare(Path.GetFileName(oldNameBase), Path.GetFileName(newNameBase), IsCaseSensitive) !=0);


@jinujoseph Can you fix this to not call into C# specific SyntaxFacts? You should create a C# and VB specific rename strategy in corresponding layers and use language specific SyntaxFacts in those projects.

mavasani · 2017-01-04T19:12:06Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Renamer.cs

        {
            IRenameStrategy[] strategies = new IRenameStrategy[] {
                new SimpleRenameStrategy(_threadingService, _userNotificationServices, _optionsSettings, _roslynServices)
            };
-            return strategies.FirstOrDefault(s => s.CanHandleRename(_oldFilePath, _newFilePath));
+
+            var compilation = await project.GetCompilationAsync().ConfigureAwait(false);


Compilation can be null for projects not supporting compilation, you should add a null check.

mavasani · 2017-01-04T19:13:08Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Renamer.cs

@@ -77,15 +77,17 @@ internal sealed class Renamer

        public Task RenameAsync(Project project)
        {
-            return GetStrategy()?.RenameAsync(project, _oldFilePath, _newFilePath) ?? Task.CompletedTask;
+            return GetStrategyAsync(project).Result?.RenameAsync(project, _oldFilePath, _newFilePath) ?? Task.CompletedTask;


use await, not .Result.

Additionally, you should do a null check as GetStrategyAsync can return null result.

mavasani · 2017-01-04T19:13:41Z

...rosoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Rename/SimpleRenameStrategy.cs

@@ -19,11 +19,11 @@ public SimpleRenameStrategy(IProjectThreadingService threadingService, IUserNoti
        }

        // For the SimpleRename, it can attempt to handle any situtation
-        public override bool CanHandleRename(string oldFileName, string newFileName)
+        public override bool CanHandleRename(string oldFileName, string newFileName, bool IsCaseSensitive)


param casing: isCaseSensitive

davkean · 2017-01-05T03:23:07Z

...rosoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Rename/SimpleRenameStrategy.cs

        {
            var oldNameBase = Path.GetFileNameWithoutExtension(oldFileName);
            var newNameBase = Path.GetFileNameWithoutExtension(newFileName);
-            return SyntaxFacts.IsValidIdentifier(oldNameBase) && SyntaxFacts.IsValidIdentifier(newNameBase);
+            return SyntaxFacts.IsValidIdentifier(oldNameBase) && SyntaxFacts.IsValidIdentifier(newNameBase) && (string.Compare(Path.GetFileName(oldNameBase), Path.GetFileName(newNameBase), isCaseSensitive) !=0);


This does a cultural aware comparison, so "encyclopedia" == "encyclopædia" and will result in different results based on the user's current culture. You are dealing with paths here, so you want an ordinal comparison.

Can you add tests cases for this? If I rename encyclopedia to encyclopædia it should still rename.

davkean

As above.

jinujoseph · 2017-01-06T09:00:56Z

@srivatsn for approval

dnfclas added the cla-already-signed label Dec 12, 2016

333fred requested changes Dec 12, 2016

View reviewed changes

333fred reviewed Dec 12, 2016

View reviewed changes

jinujoseph force-pushed the RenameFix branch 2 times, most recently from 016845b to 2ff9551 Compare December 12, 2016 23:33

davkean requested changes Dec 13, 2016

View reviewed changes

jinujoseph force-pushed the RenameFix branch from 2ff9551 to 0eeac84 Compare December 13, 2016 17:50

333fred approved these changes Dec 23, 2016

View reviewed changes

Don't trigger rename of code element if filename has not been changed.

cfc8d6d

jinujoseph force-pushed the RenameFix branch from 0eeac84 to b53f97f Compare January 3, 2017 22:47

mavasani reviewed Jan 4, 2017

View reviewed changes

jinujoseph force-pushed the RenameFix branch from b53f97f to df741a2 Compare January 4, 2017 21:36

davkean reviewed Jan 5, 2017

View reviewed changes

davkean requested changes Jan 5, 2017

View reviewed changes

Adding the language specific casesensitive check

8b6ecd7

jinujoseph force-pushed the RenameFix branch from df741a2 to 8b6ecd7 Compare January 5, 2017 08:32

davkean approved these changes Jan 5, 2017

View reviewed changes

srivatsn added the Approved for next preview label Jan 6, 2017

jinujoseph merged commit 919ce33 into dotnet:master Jan 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't trigger rename of code element if filename has not been changed. #920

Don't trigger rename of code element if filename has not been changed. #920

jinujoseph commented Dec 12, 2016 •

edited by srivatsn

333fred left a comment

333fred Dec 12, 2016

jinujoseph commented Dec 12, 2016

davkean Dec 13, 2016

davkean Dec 13, 2016

davkean Dec 13, 2016 •

edited

jinujoseph Dec 13, 2016

davkean commented Dec 15, 2016

davkean commented Dec 15, 2016

jinujoseph commented Dec 15, 2016

davkean commented Jan 3, 2017

jinujoseph commented Jan 3, 2017 •

edited

mavasani commented Jan 3, 2017

davkean commented Jan 4, 2017

jinujoseph commented Jan 4, 2017

mavasani Jan 4, 2017

mavasani Jan 4, 2017

mavasani Jan 4, 2017

mavasani Jan 4, 2017

davkean Jan 5, 2017

davkean Jan 5, 2017

davkean left a comment

jinujoseph commented Jan 6, 2017

Don't trigger rename of code element if filename has not been changed. #920

Don't trigger rename of code element if filename has not been changed. #920

Conversation

jinujoseph commented Dec 12, 2016 • edited by srivatsn

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean Dec 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean commented Dec 15, 2016

davkean commented Dec 15, 2016

jinujoseph commented Dec 15, 2016

davkean commented Jan 3, 2017

jinujoseph commented Jan 3, 2017 • edited

mavasani commented Jan 3, 2017

davkean commented Jan 4, 2017

jinujoseph commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean left a comment

Choose a reason for hiding this comment

jinujoseph commented Jan 6, 2017

jinujoseph commented Dec 12, 2016 •

edited by srivatsn

davkean Dec 13, 2016 •

edited

jinujoseph commented Jan 3, 2017 •

edited