Skip to content

Remove ES5 IntelliSense Option #1255

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

Merged
merged 7 commits into from
Sep 9, 2016
Merged

Remove ES5 IntelliSense Option #1255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f1f0c89
Remove ES5 IntelliSense Option
mjbvz Sep 8, 2016
7aadd85
Remove unused intellisense level changed logging call
mjbvz Sep 9, 2016
fe5700b
Remove two more unused options and disable intellisense page on vs15
mjbvz Sep 9, 2016
e4c20e3
Remove horizontal divider from options page
mjbvz Sep 9, 2016
116f216
Small cleanup for diagnosticscommand
mjbvz Sep 9, 2016
e3bf5ae
Remove unused TelemtryEvent name
mjbvz Sep 9, 2016
bc5800d
Remove Update contentType function
mjbvz Sep 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Nodejs/Common/Telemetry/TelemetryEvents.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,5 @@ namespace Microsoft.NodejsTools.Telemetry {
internal static class TelemetryEvents {
public const string ProjectImported = "ProjectImported";
public const string AnalysisActivatedForProject = "AnalysisActivatedForProject";
public const string AnalysisLevelChanged = "AnalysisLevelChanged";
}
}
19 changes: 1 addition & 18 deletions Nodejs/Product/Nodejs/Commands/DiagnosticsCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using System.Text;
using System.Threading;
using Microsoft.NodejsTools.Logging;
using Microsoft.NodejsTools.Options;
using Microsoft.VisualStudioTools;
using Microsoft.VisualStudioTools.Project;

Expand Down Expand Up @@ -74,7 +75,6 @@ private string GetData() {
res.AppendLine(GetSolutionInfo());
res.AppendLine(GetEventsAndStatsInfo());
res.AppendLine(GetLoadedAssemblyInfo());
res.AppendLine(GetAnalysisLevelInfo());
return res.ToString();
}

Expand Down Expand Up @@ -273,23 +273,6 @@ private static string GetLoadedAssemblyInfo() {
return res.ToString();
}

private static string GetAnalysisLevelInfo() {
var res = new StringBuilder();
res.AppendLine(String.Format("Analysis Level: {0}", NodejsPackage.Instance.IntellisenseOptionsPage.AnalysisLevel.ToString()));
res.AppendLine();
if (NodejsPackage.Instance._analyzer != null) {
var jsAnalyzer = NodejsPackage.Instance._analyzer;
res.AppendLine("Default Analysis Log: ");

using (var writer = new StringWriter(res)) {
jsAnalyzer.DumpLog(writer);
}
}

res.AppendLine(string.Format("IntelliSense Completion Only Tab or Enter to Commit: {0}", NodejsPackage.Instance.IntellisenseOptionsPage.OnlyTabOrEnterToCommit));
return res.ToString();
}

private static string Indent(int count, string text) {
var indent = new string(' ', count * 4);
var lines = text.Split(new[] { "\r\n", "\r", "\n" }, StringSplitOptions.None);
Expand Down
11 changes: 3 additions & 8 deletions Nodejs/Product/Nodejs/Intellisense/IntellisenseController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,7 @@ private void HandleChar(char ch) {
}
break;
default:
if (IsIdentifierFirstChar(ch) && _activeSession == null
&& NodejsPackage.Instance.LangPrefs.AutoListMembers
&& NodejsPackage.Instance.IntellisenseOptionsPage.ShowCompletionListAfterCharacterTyped) {
if (IsIdentifierFirstChar(ch) && _activeSession == null && NodejsPackage.Instance.LangPrefs.AutoListMembers) {
TriggerCompletionSession(false);
}
break;
Expand Down Expand Up @@ -503,9 +501,7 @@ public int Exec(ref Guid pguidCmdGroup, uint nCmdID, uint nCmdexecopt, IntPtr pv
committedBy = "\"";
}
} else {
committedBy = NodejsPackage.Instance != null && NodejsPackage.Instance.IntellisenseOptionsPage.OnlyTabOrEnterToCommit ?
string.Empty :
NodejsConstants.DefaultIntellisenseCompletionCommittedBy;
committedBy = NodejsConstants.DefaultIntellisenseCompletionCommittedBy;
}

if (committedBy.IndexOf(ch) != -1) {
Expand Down Expand Up @@ -648,8 +644,7 @@ public int Exec(ref Guid pguidCmdGroup, uint nCmdID, uint nCmdexecopt, IntPtr pv
case VSConstants.VSStd2KCmdID.COMPLETEWORD:
ForceCompletions = true;
try {
TriggerCompletionSession((VSConstants.VSStd2KCmdID)nCmdID == VSConstants.VSStd2KCmdID.COMPLETEWORD
&& !NodejsPackage.Instance.IntellisenseOptionsPage.OnlyTabOrEnterToCommit);
TriggerCompletionSession(false);
Copy link
Contributor

@mousetraps mousetraps Sep 9, 2016

Choose a reason for hiding this comment

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

still should check for VSConstants.VSSt2KCmdID.COMPLETEWORD

Copy link
Contributor Author

@mjbvz mjbvz Sep 9, 2016

Choose a reason for hiding this comment

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

OnlyTabOrEnterToCommit defaults to true in my understanding, which would make this (VSConstants.VSStd2KCmdID)nCmdID == VSConstants.VSStd2KCmdID.COMPLETEWORD && !true. Is that not correct?

Copy link
Contributor

@mousetraps mousetraps Sep 9, 2016

Choose a reason for hiding this comment

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

yep, that's actually correct - parsed it wrong at first 😃

} finally {
ForceCompletions = false;
}
Expand Down
35 changes: 0 additions & 35 deletions Nodejs/Product/Nodejs/Intellisense/VsProjectAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,41 +1075,6 @@ CollectingErrorSink errorSink
) {
// There are some scenarios during ES6 Mode where a textview is still opened using NodeLS.
// In these cases, we do not properly parse ES6 syntax, and therefore do not want to display errors.
if (NodejsPackage.Instance.IntellisenseOptionsPage.AnalysisLevel == AnalysisLevel.Preview) {
return;
}

// Update the warn-on-launch state for this entry
bool changed = false;
lock (_hasParseErrors) {
if (errorSink.Errors.Any() ? _hasParseErrors.Add(entry) : _hasParseErrors.Remove(entry)) {
changed = true;
}
}
if (changed) {
OnShouldWarnOnLaunchChanged(entry);
}

var f = new TaskProviderItemFactory(snapshot);

// Update the parser warnings/errors
if (errorSink.Warnings.Any() || errorSink.Errors.Any()) {
TaskProvider.ReplaceItems(
entry,
ParserTaskMoniker,
errorSink.Warnings
.Where(ShouldIncludeWarning)
.Select(er => f.FromParseWarning(er))
.Concat(errorSink.Errors.Select(er => f.FromParseError(er)))
.ToList()
);
} else {
TaskProvider.Clear(entry, ParserTaskMoniker);
}
#if FALSE
// Add a handler for the next complete analysis
_unresolvedSquiggles.ListenForNextNewAnalysis(entry as IJsProjectEntry);
#endif
}

private bool ShouldIncludeWarning(ErrorResult error) {
Copy link
Contributor

@mousetraps mousetraps Sep 9, 2016

Choose a reason for hiding this comment

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

nit: I would prefer to remove just this, and remove everything below in a separate, more focused change, when we remove the analyzer altogether

Copy link
Contributor Author

@mjbvz mjbvz Sep 9, 2016

Choose a reason for hiding this comment

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

Do you mean remove the entire If block? This code is unreachable after this change if we keep the current logic.

Expand Down
10 changes: 0 additions & 10 deletions Nodejs/Product/Nodejs/Nodejs.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,12 +303,6 @@
<Compile Include="Options\NodejsDiagnosticsOptionsPage.cs">
<SubType>Component</SubType>
</Compile>
<Compile Include="Options\NodeLsIntellisenseOptionsControl.cs">
<SubType>UserControl</SubType>
</Compile>
<Compile Include="Options\NodeLsIntellisenseOptionsControl.Designer.cs">
<DependentUpon>NodeLsIntellisenseOptionsControl.cs</DependentUpon>
</Compile>
<Compile Include="Options\NodejsNpmOptionsPage.cs">
<SubType>Component</SubType>
</Compile>
Expand Down Expand Up @@ -674,10 +668,6 @@
<EmbeddedResource Include="Commands\DiagnosticsForm.resx">
<DependentUpon>DiagnosticsForm.cs</DependentUpon>
</EmbeddedResource>
<EmbeddedResource Include="Options\NodeLsIntellisenseOptionsControl.resx">
<DependentUpon>NodeLsIntellisenseOptionsControl.cs</DependentUpon>
<SubType>Designer</SubType>
</EmbeddedResource>
<EmbeddedResource Include="Options\NodejsNpmOptionsControl.resx">
<DependentUpon>NodejsNpmOptionsControl.cs</DependentUpon>
</EmbeddedResource>
Expand Down
38 changes: 5 additions & 33 deletions Nodejs/Product/Nodejs/NodejsPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ namespace Microsoft.NodejsTools {
[ProvideLanguageEditorOptionPage(typeof(NodejsFormattingSpacingOptionsPage), NodejsConstants.Nodejs, "Formatting", "Spacing", "3042")]
[ProvideLanguageEditorOptionPage(typeof(NodejsFormattingBracesOptionsPage), NodejsConstants.Nodejs, "Formatting", "Braces", "3043")]
[ProvideLanguageEditorOptionPage(typeof(NodejsFormattingGeneralOptionsPage), NodejsConstants.Nodejs, "Formatting", "General", "3044")]
Copy link
Contributor

@mousetraps mousetraps Sep 9, 2016

Choose a reason for hiding this comment

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

Eventually we should remove Node.js formatting options pages as well when TS uses JS, rather than TS to back their editor. Worth adding an item to track.

Copy link
Contributor Author

@mjbvz mjbvz Sep 9, 2016

Choose a reason for hiding this comment

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

Opened #1258 to track this.

#if DEV14
[ProvideLanguageEditorOptionPage(typeof(NodejsIntellisenseOptionsPage), NodejsConstants.Nodejs, "IntelliSense", "", "3048")]
#endif
[ProvideLanguageEditorOptionPage(typeof(NodejsAdvancedEditorOptionsPage), NodejsConstants.Nodejs, "Advanced", "", "3050")]
[ProvideCodeExpansions(Guids.NodejsLanguageInfoString, false, 106, "Nodejs", @"Snippets\%LCID%\SnippetsIndex.xml", @"Snippets\%LCID%\Nodejs\")]
[ProvideCodeExpansionPath("Nodejs", "Test", @"Snippets\%LCID%\Test\")]
Expand Down Expand Up @@ -241,10 +243,6 @@ protected override void Initialize() {

MakeDebuggerContextAvailable();

IntellisenseOptionsPage.AnalysisLogMaximumChanged += IntellisenseOptionsPage_AnalysisLogMaximumChanged;
IntellisenseOptionsPage.AnalysisLevelChanged += IntellisenseOptionsPageAnalysisLevelChanged;
IntellisenseOptionsPage.SaveToDiskChanged += IntellisenseOptionsPageSaveToDiskChanged;

InitializeLogging();

InitializeTelemetry();
Expand Down Expand Up @@ -282,19 +280,12 @@ private void SubscribeToVsCommandEvents(
_subscribedCommandEvents.Add(targetEvent);
}


private void IntellisenseOptionsPage_AnalysisLogMaximumChanged(object sender, EventArgs e) {
if (_analyzer != null) {
_analyzer.MaxLogLength = IntellisenseOptionsPage.AnalysisLogMax;
}
}

private void InitializeLogging() {
_logger = new NodejsToolsLogger(ComponentModel.GetExtensions<INodejsToolsLogger>().ToArray());

// log interesting stats on startup
_logger.LogEvent(NodejsToolsLogEvent.SurveyNewsFrequency, GeneralOptionsPage.SurveyNewsCheck);
_logger.LogEvent(NodejsToolsLogEvent.AnalysisLevel, IntellisenseOptionsPage.AnalysisLevel);
_logger.LogEvent(NodejsToolsLogEvent.AnalysisLevel, AnalysisLevel.Preview);
}

private void InitializeTelemetry() {
Expand Down Expand Up @@ -603,31 +594,12 @@ internal VsProjectAnalyzer DefaultAnalyzer {
if (_analyzer == null) {
_analyzer = CreateLooseVsProjectAnalyzer();
LogLooseFileAnalysisLevel();
_analyzer.MaxLogLength = IntellisenseOptionsPage.AnalysisLogMax;
_analyzer.MaxLogLength = 100;
}
return _analyzer;
}
}

private void IntellisenseOptionsPageSaveToDiskChanged(object sender, EventArgs e) {
if (_analyzer != null) {
_analyzer.SaveToDisk = IntellisenseOptionsPage.SaveToDisk;
}
}

private void IntellisenseOptionsPageAnalysisLevelChanged(object sender, EventArgs e) {
if (_analyzer != null) {
var analyzer = CreateLooseVsProjectAnalyzer();
analyzer.SwitchAnalyzers(_analyzer);
if (_analyzer.RemoveUser()) {
_analyzer.Dispose();
}
_analyzer = analyzer;
LogLooseFileAnalysisLevel();
}
TelemetryLogger.LogAnalysisLevelChanged(IntellisenseOptionsPage.AnalysisLevel);
}

private VsProjectAnalyzer CreateLooseVsProjectAnalyzer() {
// In ES6 IntelliSense mode, rather than overriding the default JS editor,
// we throw to Salsa. If Salsa detects that it's not operating within the Node.js project context,
Expand All @@ -639,7 +611,7 @@ private VsProjectAnalyzer CreateLooseVsProjectAnalyzer() {
// we do not throw to Salsa from the REPL window. However, in order to provide a workable editing
// experience within the REPL context, we initialize the loose analyzer with Quick IntelliSense
// during ES6 mode.
var analysisLevel = IntellisenseOptionsPage.AnalysisLevel == AnalysisLevel.Preview ? AnalysisLevel.NodeLsMedium : IntellisenseOptionsPage.AnalysisLevel;
var analysisLevel = AnalysisLevel.NodeLsMedium;
return new VsProjectAnalyzer(analysisLevel, false);
}

Expand Down
13 changes: 4 additions & 9 deletions Nodejs/Product/Nodejs/NodejsProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ private int OpenWithNodejsEditor(uint selectionItemId) {
var properties = GetExtensionObject(_innerVsHierarchy, selectionItemId).Properties;
try {
var itemType = properties.Item("ItemType").Value;
ourEditor = (string)itemType == ProjectFileConstants.Compile && !IsES6IntellisensePreview() ? Guids.NodejsEditorFactory : Guid.Empty;
ourEditor = Guid.Empty;
} catch (ArgumentException) {
// no item type, file is excluded from project.
ourEditor = Guids.NodejsEditorFactory;
Copy link
Contributor

@mousetraps mousetraps Sep 9, 2016

Choose a reason for hiding this comment

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

Also remove the itemType assignment, otherwise we might still fall back to NodejsEditorFactory. With these changes, it may also be helpful to place a TODO or track in some other way because much of this logic is actually unnecessary since we are always using the default editor

Copy link
Contributor Author

@mjbvz mjbvz Sep 9, 2016

Choose a reason for hiding this comment

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

I've got another change that builds on this one to remove the node editors entirely (you can still reach them using Open With). I'll make sure this function is updated as part of that work.

Expand Down Expand Up @@ -572,11 +572,6 @@ out frame
return hr;
}

private bool IsES6IntellisensePreview() {
// If the analysis level is set to preview then use the TypeScript language service for node.js
return NodejsPackage.Instance.IntellisenseOptionsPage.AnalysisLevel == Options.AnalysisLevel.Preview;
}

#region IVsProject Members

public int AddItem(uint itemidLoc, VSADDITEMOPERATION dwAddItemOperation, string pszItemName, uint cFilesToOpen, string[] rgpszFilesToOpen, IntPtr hwndDlgOwner, VSADDRESULT[] pResult) {
Expand All @@ -600,7 +595,7 @@ public int AddItem(uint itemidLoc, VSADDITEMOPERATION dwAddItemOperation, string
}

if (!isClientCode && _innerProject3 != null && IsJavaScriptFile(pszItemName)) {
Guid ourEditor = IsES6IntellisensePreview() ? Guid.Empty : typeof(NodejsEditorFactory).GUID;
Guid ourEditor = Guid.Empty;
Guid view = Guid.Empty;
return _innerProject3.AddItemWithSpecific(
itemidLoc,
Expand Down Expand Up @@ -640,7 +635,7 @@ public int IsDocumentInProject(string pszMkDocument, out int pfFound, VSDOCUMENT
public int OpenItem(uint itemid, ref Guid rguidLogicalView, IntPtr punkDocDataExisting, out IVsWindowFrame ppWindowFrame) {
if (_innerProject3 != null && IsJavaScriptFile(GetItemName(_innerVsHierarchy, itemid))) {
// force .js files opened w/o an editor type to be opened w/ our editor factory.
Guid guid = IsES6IntellisensePreview() ? Guid.Empty : typeof(NodejsEditorFactory).GUID;
Guid guid = Guid.Empty;
Guid view = Guid.Empty;
int hr = _innerProject3.OpenItemWithSpecific(
itemid,
Expand Down Expand Up @@ -675,7 +670,7 @@ public int ReopenItem(uint itemid, ref Guid rguidEditorType, string pszPhysicalV
// force .js files opened w/o an editor type to be opened w/ our editor factory.
// If the item type of this file is not compile, we don't actually want to open with Nodejs and should instead use the default.
var itemType = GetExtensionObject(_innerVsHierarchy, itemid).Properties.Item("ItemType").Value;
Guid guid = (string)itemType == ProjectFileConstants.Compile && !IsES6IntellisensePreview() ? Guids.NodejsEditorFactory : Guid.Empty;
Guid guid = Guid.Empty;

return _innerProject3.ReopenItem(
itemid,
Expand Down
Loading