Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix a memory leak in _CPImageAndTextView. This closes #1888. #1889

Merged
merged 1 commit into from

4 participants

Saikat Chakrabarti CappBot Andrew Hankinson Alexander Ljungberg
Saikat Chakrabarti

No description provided.

CappBot
Collaborator

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

Alexander Ljungberg aljungberg commented on the diff
AppKit/_CPImageAndTextView.j
@@ -319,9 +319,6 @@ var _CPimageAndTextViewFrameSizeChangedFlag = 1 << 0,
if (_image == anImage)
return;
- if ([_image delegate] === self)
- [[CPNotificationCenter defaultCenter] removeObserver:self name:CPImageDidLoadNotification object:_image];
-
Alexander Ljungberg Owner

Even if we do remove the observer in imageDidLoad:, don't we want to remove the observer here too in case setImage: is called with a different image before imageDidLoad: occurs?

Saikat Chakrabarti
saikat added a note

Yes, I think you are correct. There will need to be a removeObserver before line 322. Should I add this as a commit to this pull request?

Alexander Ljungberg Owner

Yeah just add another commit, that'd be great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Andrew Hankinson

-#new
+bug
+AppKit
+#needs-review

CappBot
Collaborator

Milestone: Someday. Labels: #needs-review, AppKit, bug. What's next? This issue is pending an architectural or implementation design decision and should be discussed or voted on.

Andrew Hankinson

@saikat Could you please respond to @aljungberg's comment? Should that section be deleted, or can it be re-added without any problems? Thanks.

Alexander Ljungberg aljungberg merged commit aa0602d into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 31, 2013
  1. Saikat Chakrabarti
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 3 deletions.
  1. +1 −3 AppKit/_CPImageAndTextView.j
4 AppKit/_CPImageAndTextView.j
View
@@ -319,9 +319,6 @@ var _CPimageAndTextViewFrameSizeChangedFlag = 1 << 0,
if (_image == anImage)
return;
- if ([_image delegate] === self)
- [[CPNotificationCenter defaultCenter] removeObserver:self name:CPImageDidLoadNotification object:_image];
-
Alexander Ljungberg Owner

Even if we do remove the observer in imageDidLoad:, don't we want to remove the observer here too in case setImage: is called with a different image before imageDidLoad: occurs?

Saikat Chakrabarti
saikat added a note

Yes, I think you are correct. There will need to be a removeObserver before line 322. Should I add this as a commit to this pull request?

Alexander Ljungberg Owner

Yeah just add another commit, that'd be great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
_image = anImage;
_flags |= _CPImageAndTextViewImageChangedFlag;
@@ -347,6 +344,7 @@ var _CPimageAndTextViewFrameSizeChangedFlag = 1 << 0,
- (void)imageDidLoad:(CPNotification)aNotification
{
+ [[CPNotificationCenter defaultCenter] removeObserver:self name:CPImageDidLoadNotification object:_image];
_flags |= _CPImageAndTextViewImageChangedFlag;
[self setNeedsLayout];
}
Something went wrong with that request. Please try again.