Skip to content
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

Jump to bottom

[TIMOB-24086] iOS: Label doesn't fire link event correctly #8568

Merged
merged 1 commit into from Nov 7, 2016
Merged

[TIMOB-24086] iOS: Label doesn't fire link event correctly #8568

merged 1 commit into from Nov 7, 2016

Conversation

kopiro
Copy link
Contributor

@kopiro kopiro commented Oct 31, 2016 •
edited by hansemannn

@build
Copy link
Contributor

build commented Oct 31, 2016

Can one of the admins verify this patch?

@kopiro kopiro changed the title New method to get correct character index [AC-4587] iOS: New method to get correct character index Oct 31, 2016
@kopiro kopiro changed the title [AC-4587] iOS: New method to get correct character index [AC-4587] iOS: Labels doesn't fire link event correctly Oct 31, 2016
@kopiro kopiro changed the title [AC-4587] iOS: Labels doesn't fire link event correctly [AC-4587] iOS: Label doesn't fire link event correctly Oct 31, 2016
@sgtcoolguy
Copy link
Contributor

ok to test

@sgtcoolguy sgtcoolguy added this to the 6.1.0 milestone Oct 31, 2016
@hansemannn hansemannn changed the title [AC-4587] iOS: Label doesn't fire link event correctly [TIMOB-24086] iOS: Label doesn't fire link event correctly Oct 31, 2016
hansemannn
hansemannn requested changes Oct 31, 2016
View reviewed changes
Copy link
Collaborator

@hansemannn hansemannn left a comment •
edited

Choose a reason for hiding this comment

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

Added some comments. Please also provide the test-case from the video in the JIRA.

Please check this leaks as well:

screen shot 2016-11-01 at 12 15 21

EDIT: Proposal for fixing the leaks (untested so far): https://gist.github.com/hansemannn/eb81b09926a64b208268741da4cf3711

iphone/Classes/TiUILabel.m
NSString *url = nil;
NSRange theRange = NSMakeRange(0, 0);
NSString *url = nil;
NSString *url2 = nil;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, sorry, typo.

iphone/Classes/TiUILabel.m
url, @"url",
[NSArray arrayWithObjects:NUMUINTEGER(theRange.location), NUMUINTEGER(theRange.length),nil],@"range",
nil];
[[self proxy] fireEvent:@"link" withObject:eventDict propagate:NO reportSuccess:NO errorCode:0 message:nil];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a check to see if the event-listener is added, e.g. [[self proxy] _hasListeners:@"link"]. It may be checked even before to prevent unnecessary memory usage. Maybe in the method call inside recognizedTap:.

And finally, I noticed the method returns a bool but it's callee does not use the return-value. I never worked on that API before, can you think of we that was done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansemannn Is already checked on the first line of recognizedTap:

    BOOL testLink = (label != nil) &&([(TiViewProxy*)[self proxy] _hasListeners:@"link" checkParent:NO]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansemannn Yes, its boolean value is never used (NOW).
But I think that we should keep the method signature as a bool just... because we can.

iphone/Classes/TiUILabel.m
textContainer.lineFragmentPadding = 0;
textContainer.maximumNumberOfLines = (NSUInteger)self.label.numberOfLines;
textContainer.lineBreakMode = self.label.lineBreakMode;
return textContainer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still will probably leak, did you run the Analyser already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Analyser?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Xcode Analyzer, great tool to find memory leaks in your code without running the app:
ev3wb

@hansemannn
Copy link
Collaborator

@kopiro Let me know when you applied the changes, the FT works fine!

@kopiro kopiro force-pushed the PR-AC-4587 branch 5 times, most recently from cbd97fc to a92be49 Compare November 7, 2016 09:12
@kopiro
Copy link
Contributor Author

kopiro commented Nov 7, 2016 •
edited

@hansemannn Implemented but there was a [textStorage release] that bricked thinks.

Implemented as a RELEASE_TO_NIL before return. (a92be49#diff-2c9944d65132833cd5df17dcdd3ebc3aR256)

Tested.

@hansemannn
Copy link
Collaborator

Ohh, nice! PR approved 🚀

@hansemannn hansemannn merged commit 469026f into tidev:master Nov 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hansemannn hansemannn hansemannn requested changes

Assignees

@hansemannn hansemannn

Labels
bug community 🔥 ios
Projects
None yet
Milestone
6.1.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kopiro @build @sgtcoolguy @hansemannn @vijaysingh-axway