-
Notifications
You must be signed in to change notification settings - Fork 301
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.
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
Clean COM wrappers leak: Round 2 #4491
Changes from all commits
5539581
b03a59d
14665a3
9bfbca0
84503b7
1d1b55a
06ae0be
71034ff
869f783
599e852
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,8 +169,27 @@ public void EvaluateCanExecute(RubberduckParserState state) | |
} | ||
} | ||
|
||
public ICommandBars Parent { get; set; } | ||
public ICommandBar Item { get; private set; } | ||
private ICommandBars _parent; | ||
public ICommandBars Parent | ||
{ | ||
get => _parent; | ||
set | ||
{ | ||
_parent?.Dispose(); | ||
_parent = value; | ||
} | ||
} | ||
|
||
private ICommandBar _item; | ||
public ICommandBar Item | ||
{ | ||
get => _item; | ||
private set | ||
{ | ||
_item?.Dispose(); | ||
_item = value; | ||
} | ||
} | ||
|
||
public void RemoveCommandBar() | ||
{ | ||
|
@@ -181,9 +200,9 @@ public void RemoveCommandBar() | |
Logger.Debug("Removing commandbar."); | ||
RemoveChildren(); | ||
Item.Delete(); | ||
Item.Dispose(); | ||
|
||
// Setting them to null will automatically dispose those | ||
Item = null; | ||
Parent?.Dispose(); | ||
Parent = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we get a comment here that nulling them out also disposes them? That helps me not remove these lines |
||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,8 +24,27 @@ protected ParentMenuItemBase(string key, IEnumerable<IMenuItem> items, int? befo | |
_items = items.ToDictionary(item => item, item => null as ICommandBarControl); | ||
} | ||
|
||
public ICommandBarControls Parent { get; set; } | ||
public ICommandBarPopup Item { get; private set; } | ||
private ICommandBarControls _parent; | ||
public ICommandBarControls Parent | ||
{ | ||
get => _parent; | ||
set | ||
{ | ||
_parent?.Dispose(); | ||
_parent = value; | ||
} | ||
} | ||
|
||
private ICommandBarPopup _item; | ||
public ICommandBarPopup Item | ||
{ | ||
get => _item; | ||
private set | ||
{ | ||
_item?.Dispose(); | ||
_item = value; | ||
} | ||
} | ||
|
||
public string Key => Item?.Tag; | ||
|
||
|
@@ -91,7 +110,8 @@ public void RemoveMenu() | |
Logger.Debug($"Removing menu {_key}."); | ||
RemoveChildren(); | ||
Item?.Delete(); | ||
Item?.Dispose(); | ||
|
||
//This will also dispose the Item as well | ||
Item = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above re: the comment. Why is the Parent not disposed / nulled here? |
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these after the flicking the
_disposed
switch? That feels a little wrongThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that was what we were supposed to -- if we throw during the dispose, the pooch is totally screwed anyway? At least subsequent calls would not throw again (but we're still screwed).