From 2a826a7a5fb8ced2f19777d205dc026cf88261f5 Mon Sep 17 00:00:00 2001 From: mkonicek Date: Mon, 6 Dec 2010 15:44:38 +0100 Subject: [PATCH] Editor context actions - explicitly setting EditorContext.Editor = null from ContextActionsRenderer when the ViewModel is discarded. Prevents leaks of reference to CodeEditorView when IContextActionProviders keep long-lived references to EditorContext. (thanks to Daniel!) --- .../Src/ContextActionsRenderer.cs | 9 ++ .../Project/SharpRefactoring.csproj | 1 - .../Src/ContextActions/CacheClassAtCaret.cs | 4 +- .../Src/ContextActions/DelegateAction.cs | 30 ---- .../Project/Src/ContextActions/ParamCheck.cs | 19 +-- .../ContextActions/ContextAction.cs | 6 +- .../ContextActions/ContextActionsService.cs | 8 +- .../ContextActions/EditorContext.cs | 140 +++++++++++------- .../ContextActions/IContextAction.cs | 8 +- .../ContextActions/IContextActionCache.cs | 2 +- 10 files changed, 122 insertions(+), 105 deletions(-) delete mode 100644 src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/DelegateAction.cs diff --git a/src/AddIns/DisplayBindings/AvalonEdit.AddIn/Src/ContextActionsRenderer.cs b/src/AddIns/DisplayBindings/AvalonEdit.AddIn/Src/ContextActionsRenderer.cs index a30362582a..1d9f98d0bd 100644 --- a/src/AddIns/DisplayBindings/AvalonEdit.AddIn/Src/ContextActionsRenderer.cs +++ b/src/AddIns/DisplayBindings/AvalonEdit.AddIn/Src/ContextActionsRenderer.cs @@ -107,10 +107,13 @@ namespace ICSharpCode.AvalonEdit.AddIn this.popup.ViewModel = popupVM; this.popup.OpenAtLineStart(this.Editor); } + + EditorActionsProvider lastActions; ContextActionsBulbViewModel BuildPopupViewModel(ITextEditor editor) { var actionsProvider = ContextActionsService.Instance.GetAvailableActions(editor); + this.lastActions = actionsProvider; return new ContextActionsBulbViewModel(actionsProvider); } @@ -130,6 +133,11 @@ namespace ICSharpCode.AvalonEdit.AddIn this.popup.IsDropdownOpen = false; this.popup.IsHiddenActionsExpanded = false; this.popup.ViewModel = null; + if (this.lastActions != null) { + // Clear the context to prevent memory leaks in case some users kept long-lived references to EditorContext + this.lastActions.EditorContext.Clear(); + this.lastActions = null; + } } void WorkbenchSingleton_Workbench_ViewClosed(object sender, ViewContentEventArgs e) @@ -139,6 +147,7 @@ namespace ICSharpCode.AvalonEdit.AddIn if (e.Content.PrimaryFileName == this.Editor.FileName) { WorkbenchSingleton.Workbench.ViewClosed -= WorkbenchSingleton_Workbench_ViewClosed; WorkbenchSingleton.Workbench.ActiveViewContentChanged -= WorkbenchSingleton_Workbench_ActiveViewContentChanged; + ClosePopup(); } } catch {} } diff --git a/src/AddIns/Misc/SharpRefactoring/Project/SharpRefactoring.csproj b/src/AddIns/Misc/SharpRefactoring/Project/SharpRefactoring.csproj index c1afb0c6e8..407caf96d5 100644 --- a/src/AddIns/Misc/SharpRefactoring/Project/SharpRefactoring.csproj +++ b/src/AddIns/Misc/SharpRefactoring/Project/SharpRefactoring.csproj @@ -77,7 +77,6 @@ - diff --git a/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/CacheClassAtCaret.cs b/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/CacheClassAtCaret.cs index a52a8eebb0..31f08e2f4c 100644 --- a/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/CacheClassAtCaret.cs +++ b/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/CacheClassAtCaret.cs @@ -12,7 +12,8 @@ using ICSharpCode.SharpDevelop.Refactoring; namespace SharpRefactoring.ContextActions { /// - /// Description of CacheClassAtCaret. + /// Caches information about the class at the caret in the editor. + /// Used by ContextActions. /// public class CacheClassAtCaret : IContextActionCache { @@ -50,7 +51,6 @@ namespace SharpRefactoring.ContextActions return; var c = this.Class; - // TODO cache var classDecls = context.GetClassDeclarationsOnCurrentLine().ToList(); this.IsCaretAtClassDeclaration = classDecls.Count == 1 && (classDecls[0].FullyQualifiedName == c.FullyQualifiedName); diff --git a/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/DelegateAction.cs b/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/DelegateAction.cs deleted file mode 100644 index 3cba2e06f0..0000000000 --- a/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/DelegateAction.cs +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) AlphaSierraPapa for the SharpDevelop Team (for details please see \doc\copyright.txt) -// This code is distributed under the GNU LGPL (for details please see \doc\license.txt) - -using System; -using System.Collections.Generic; -using System.Linq; -using System.Windows; -using ICSharpCode.NRefactory; -using Ast = ICSharpCode.NRefactory.Ast; -using ICSharpCode.SharpDevelop; -using ICSharpCode.SharpDevelop.Dom; -using ICSharpCode.SharpDevelop.Dom.NRefactoryResolver; -using ICSharpCode.SharpDevelop.Editor; -using ICSharpCode.SharpDevelop.Editor.AvalonEdit; -using ICSharpCode.SharpDevelop.Refactoring; - -namespace SharpRefactoring.ContextActions -{ - public class DelegateAction : IContextAction - { - public string Title { get; set; } - public System.Action ExecuteAction { get; set; } - - public void Execute() - { - if (this.ExecuteAction != null) - this.ExecuteAction(); - } - } -} diff --git a/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/ParamCheck.cs b/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/ParamCheck.cs index 38197440eb..f4014249c1 100644 --- a/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/ParamCheck.cs +++ b/src/AddIns/Misc/SharpRefactoring/Project/Src/ContextActions/ParamCheck.cs @@ -12,6 +12,15 @@ namespace SharpRefactoring.ContextActions /// public abstract class ParamCheck : ContextAction { + public override bool IsAvailable(EditorContext context) + { + var paramAtCaret = GetParameterAtCaret(context.CurrentSymbol); + if (paramAtCaret == null) + return false; + + return IsAvailable(paramAtCaret.ResolvedType); + } + public LocalResolveResult GetParameterAtCaret(ResolveResult symbol) { LocalResolveResult param = symbol as LocalResolveResult; @@ -21,17 +30,9 @@ namespace SharpRefactoring.ContextActions return null; if (!param.IsParameter) return null; + // FIXME must be parameter definition, and method with body (not interface or abstract) return param; } - - public override bool IsAvailable(EditorContext context) - { - var paramAtCaret = GetParameterAtCaret(context.CurrentSymbol); - if (paramAtCaret == null) - return false; - - return IsAvailable(paramAtCaret.ResolvedType); - } public override void Execute(EditorContext context) { diff --git a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextAction.cs b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextAction.cs index 9c26b82d56..513e607472 100644 --- a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextAction.cs +++ b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextAction.cs @@ -19,17 +19,19 @@ namespace ICSharpCode.SharpDevelop.Refactoring public abstract void Execute(EditorContext context); + public EditorContext Context { get; private set; } + public IEnumerable GetAvailableActions(EditorContext context) { + // re-initialize the context this.Context = context; if (this.IsAvailable(context)) yield return this; } - public EditorContext Context { get; private set; } - public void Execute() { + // context was re-initialized in GetAvailableActions Execute(this.Context); } } diff --git a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextActionsService.cs b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextActionsService.cs index 9a4506d666..24a0507de7 100644 --- a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextActionsService.cs +++ b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/ContextActionsService.cs @@ -54,12 +54,14 @@ namespace ICSharpCode.SharpDevelop.Refactoring } } - + /// + /// Returned by . + /// public class EditorActionsProvider { ITextEditor editor { get; set; } IList providers { get; set; } - EditorContext context { get; set; } + public EditorContext EditorContext { get; set; } public EditorActionsProvider(ITextEditor editor, IList providers) { @@ -70,7 +72,7 @@ namespace ICSharpCode.SharpDevelop.Refactoring this.editor = editor; this.providers = providers; ParserService.ParseCurrentViewContent(); - this.context = new EditorContext(editor); + this.EditorContext = new EditorContext(editor); } public IEnumerable GetVisibleActions() diff --git a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/EditorContext.cs b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/EditorContext.cs index 32e37b15ba..e5287d8a87 100644 --- a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/EditorContext.cs +++ b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/EditorContext.cs @@ -16,9 +16,9 @@ using ICSharpCode.SharpDevelop.Editor; namespace ICSharpCode.SharpDevelop.Refactoring { /// - /// Helper class for . - /// Never keep long-lived references to this class - /// - the AST serves as one-time cache and does not get updated when editor text changes. + /// Contains information about code around the caret in the editor - useful for implementing Context actions. + /// Do not keep your own references to EditorContext. + /// It serves as one-time cache and does not get updated when editor text changes. /// public class EditorContext { @@ -27,20 +27,20 @@ namespace ICSharpCode.SharpDevelop.Refactoring int CaretColumn { get; set; } /// - /// Language independent. + /// The expression at editor caret. Language independent. /// public ExpressionResult CurrentExpression { get; private set; } /// - /// Language independent. + /// The resolved symbol at editor caret. Language independent. /// public ResolveResult CurrentSymbol { get; private set; } /// - /// Language independent. + /// ParseInformation for current file. Language independent. /// public ParseInformation CurrentParseInformation { get; private set; } - public IProjectContent ProjectContent { + public IProjectContent ProjectContent { get { if (CurrentParseInformation != null) return CurrentParseInformation.CompilationUnit.ProjectContent; @@ -49,22 +49,33 @@ namespace ICSharpCode.SharpDevelop.Refactoring } } + /// + /// The editor line containing the caret. + /// public IDocumentLine CurrentLine { get; private set; } /// - /// Only available for C# and VB. + /// Parsed AST of the current editor line. Only available for C# and VB. /// public INode CurrentLineAST { get; private set; } /// - /// Only available for C# and VB. + /// Parsed AST of the member containing the editor caret. Only available for C# and VB. /// public INode CurrentMemberAST { get; private set; } /// - /// Only available for C# and VB. + /// Parsed AST of the element at editor caret. Only available for C# and VB. /// - public INode CurrentElement { get; private set; } + INode CurrentElement { get; set; } NRefactoryResolver Resolver { get; set; } + /// + /// Caches values shared by Context actions. Used in + /// + Dictionary cachedValues = new Dictionary(); + + /// + /// Fully initializes the EditorContext. + /// public EditorContext(ITextEditor editor) { if (editor == null) @@ -93,27 +104,43 @@ namespace ICSharpCode.SharpDevelop.Refactoring // DebugLog(); } - void DebugLog() + /// + /// Do not call from your Context actions - used by SharpDevelop. + /// Sets contents of editor context to null to prevent memory leaks. Used in case users implementing IContextActionProvider + /// keep long-lived references to EditorContext even when warned not to do so. + /// + public void Clear() { - ICSharpCode.Core.LoggingService.Debug(string.Format( - @" - - Context actions : - ExprAtCaret: {0} - ---------------------- - SymbolAtCaret: {1} - ---------------------- - CurrentLineAST: {2} - ---------------------- - AstNodeAtCaret: {3} - ---------------------- - CurrentMemberAST: {4} - ----------------------", - CurrentExpression, CurrentSymbol, CurrentLineAST, - CurrentElement == null ? "" : CurrentElement.ToString().TakeStartEllipsis(400), - CurrentMemberAST == null ? "" : CurrentMemberAST.ToString().TakeStartEllipsis(400))); + this.Editor = null; + this.CurrentElement = null; + this.CurrentLineAST = null; + this.CurrentMemberAST = null; + this.CurrentParseInformation = null; + this.CurrentSymbol = null; + this.Resolver = null; + this.cachedValues.Clear(); } + /// + /// Gets cached value shared by context actions. Initializes a new value if not present. + /// + public T GetCached() where T : IContextActionCache, new() + { + Type t = typeof(T); + if (cachedValues.ContainsKey(t)) { + return (T)cachedValues[t]; + } else { + T cached = new T(); + cached.Initialize(this); + cachedValues[t] = cached; + return cached; + } + } + + /// + /// Checks if the caret is exactly at AST node of given type (e.g. "if" of IfElseStatement). + /// If yes, returns it. If not, returns null. + /// public TNode GetCurrentElement() where TNode : class, INode { if (this.CurrentElement is TNode) @@ -121,6 +148,10 @@ namespace ICSharpCode.SharpDevelop.Refactoring return null; } + /// + /// Checks if caret is within AST node of given type (e.g. anywhere inside IfElseStatement). + /// If yes, returns it. If not, returns null. + /// public TNode GetContainingElement() where TNode : class, INode { var node = this.CurrentElement; @@ -133,22 +164,28 @@ namespace ICSharpCode.SharpDevelop.Refactoring return null; } - Dictionary cachedValues = new Dictionary(); - - public T GetCached() where T : IContextActionCache, new() + void DebugLog() { - Type t = typeof(T); - if (cachedValues.ContainsKey(t)) { - return (T)cachedValues[t]; - } else { - T cached = new T(); - cached.Initialize(this); - cachedValues[t] = cached; - return cached; - } + ICSharpCode.Core.LoggingService.Debug(string.Format( + @" + + Context actions : + ExprAtCaret: {0} + ---------------------- + SymbolAtCaret: {1} + ---------------------- + CurrentLineAST: {2} + ---------------------- + AstNodeAtCaret: {3} + ---------------------- + CurrentMemberAST: {4} + ----------------------", + CurrentExpression, CurrentSymbol, CurrentLineAST, + CurrentElement == null ? "" : CurrentElement.ToString().TakeStartEllipsis(400), + CurrentMemberAST == null ? "" : CurrentMemberAST.ToString().TakeStartEllipsis(400))); } - public static INode FindInnermostNode(INode node, Location position) + static INode FindInnermostNode(INode node, Location position) { if (node == null) return null; @@ -214,12 +251,7 @@ namespace ICSharpCode.SharpDevelop.Refactoring var snippetParser = GetSnippetParser(editor); if (snippetParser == null) return null; - //try { - return snippetParser.Parse(currentLine.Text); - //} - //catch { - // return null; - //} + return snippetParser.Parse(currentLine.Text); } SnippetParser GetSnippetParser(ITextEditor editor) @@ -245,14 +277,10 @@ namespace ICSharpCode.SharpDevelop.Refactoring INode GetCurrentMemberAST(ITextEditor editor) { - //try { - var resolver = GetInitializedNRefactoryResolver(editor, this.CaretLine, this.CaretColumn); - if (resolver == null) - return null; - return resolver.ParseCurrentMember(editor.Document.Text); - //} catch { - // return null; - //} + var resolver = GetInitializedNRefactoryResolver(editor, this.CaretLine, this.CaretColumn); + if (resolver == null) + return null; + return resolver.ParseCurrentMember(editor.Document.Text); } NRefactoryResolver GetInitializedNRefactoryResolver(ITextEditor editor, int caretLine, int caretColumn) diff --git a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextAction.cs b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextAction.cs index c31ef1c96b..624fe816d0 100644 --- a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextAction.cs +++ b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextAction.cs @@ -7,11 +7,17 @@ using System.Windows; namespace ICSharpCode.SharpDevelop.Refactoring { /// - /// Description of ContextAction. + /// One editor Context action. /// public interface IContextAction { + /// + /// Title displayed in the context actions popup. + /// string Title { get; } + /// + /// Executes this action. Called when this action is selected from the context actions popup. + /// void Execute(); } } diff --git a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextActionCache.cs b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextActionCache.cs index 4d5221d748..432cd83cb7 100644 --- a/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextActionCache.cs +++ b/src/Main/Base/Project/Src/Services/RefactoringService/ContextActions/IContextActionCache.cs @@ -6,7 +6,7 @@ using System; namespace ICSharpCode.SharpDevelop.Refactoring { /// - /// Data shared by multiple Context actions. Stored in EditorContext.GetCached(). + /// Temporary data shared by multiple Context actions. Stored in EditorContext.GetCached(). /// public interface IContextActionCache {