Skip to content
This repository was archived by the owner on Nov 22, 2020. It is now read-only.

Fixing a few errors, adding support for attributedPlaceholder #5

Closed
wants to merge 10 commits into from

Conversation

sam-w
Copy link

@sam-w sam-w commented Aug 6, 2014

No description provided.

Sam Warner added 6 commits August 6, 2014 15:50
- Replacing nil string with @""
- Rejecting UITextFieldTextDidChangeNotification when not this object
- Updating _storedText in all cases, not just sometimes
- Animating the float label in all cases, not just sometimes
[self toggleFloatLabel:UIFloatLabelAnimationTypeHide];
assert([notification.name isEqualToString:UITextFieldTextDidChangeNotification]);

if (notification.object == self) {
Copy link
Author

Choose a reason for hiding this comment

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

This check is needed if more than one UIFloatLabelTextField exists at the same time

Copy link
Owner

Choose a reason for hiding this comment

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

Why is this check needed? I have a sample app another user has sent me with a TableView that has dozens of UIFloatLabelTextFields. They work just fine.

Copy link
Author

Choose a reason for hiding this comment

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

Look at the code before and after. Before, we weren't rejecting notifications that were generated by objects other than ourselves, now we are.

You can see the effectiveness by putting say NSLog(@"0x%x UITextFieldTextDidChangeNotification", self); in this function. Without the check, you'll see all of the existing UIFloatLabelTextFields do something with the notification. With the check, only the UIFloatLabelTextField which actually had some text change will do something.

It may well have worked in the sample app, but it is definitely incorrect.

Copy link
Owner

Choose a reason for hiding this comment

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

ok, so you're saying that all UIFloatLabelTextField objects, even ones that are not being interacted with at a given moment are calling textDidChange:? If so, you discovered, or reaffirmed your hypothesis by checking the memory addresses of objects that are printed out in the log?

Just want to make sure before I take a deeper dive.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, correct - sorry left a bit out of the NSLog example above. Have edited the comment :)

Copy link
Owner

Choose a reason for hiding this comment

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

ok, I confirmed that this issue exists. Checking your solution now.

Copy link
Owner

Choose a reason for hiding this comment

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

so, the new textDidChange: method I have looks like this:

- (void)textDidChange:(NSNotification *)notification
{
    assert([notification.name isEqualToString:UITextFieldTextDidChangeNotification]);
    if (notification.object == self) {
        if ([self.text length]) {
            _storedText = [self text];
            if (![_floatLabel alpha]) {
                [self toggleFloatLabel:UIFloatLabelAnimationTypeShow];
            }
        } else {
            if ([_floatLabel alpha]) {
                [self toggleFloatLabel:UIFloatLabelAnimationTypeHide];
            }
        }
    }
}

I kept my conditionals, but wrapped them around your notification.object conditional check.

Copy link
Author

Choose a reason for hiding this comment

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

I think that this may well introduce more bugs - notice that storedText is only updated if [self.text length] > 0. If self.text has just changed to @"", _storedText will still contain whatever was there before.

My addition of != (_storedText.length > 0) is an efficiency measure to avoid [self toggleFloatLabel:] being called every time the text changes. With this addition, it only gets called when self.text either changes from @"" to @"something" or from @"something" to @"".

Copy link
Owner

Choose a reason for hiding this comment

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

This may be an issue due to an oversight in the original code, where I didn't reset _storedText = @"" in the else. I think that would avoid the issue outright. I need to think about it for a bit.

    if ([self.text length]) {
        _storedText = [self text];
        if (![_floatLabel alpha]) {
            [self toggleFloatLabel:UIFloatLabelAnimationTypeShow];
        }
    } else {
        if ([_floatLabel alpha]) {
            [self toggleFloatLabel:UIFloatLabelAnimationTypeHide];
        }
        _storedText = @""; // Added this now for the example
    }

@ArtSabintsev
Copy link
Owner

Hey - thanks for these changes! Will take a look today!

@ArtSabintsev
Copy link
Owner

There's actually another small issue I need to fix first before mulling through this request. Working on that now.

@ArtSabintsev
Copy link
Owner

Fixed that issue. Going to push it now.

@sam-w
Copy link
Author

sam-w commented Aug 7, 2014

No worries :)

@@ -15,6 +15,7 @@ @interface UIFloatLabelTextField ()
@property (nonatomic, assign) CGFloat xOrigin;
@property (nonatomic, assign) CGFloat horizontalPadding;
@property (nonatomic, assign) CGFloat verticalPadding;
@property NSDictionary *placeholderAttributes;
Copy link
Owner

Choose a reason for hiding this comment

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

I know that @Property declarations default to (nonatomic, strong), but for the sake of code readability - could you add them in in your fork?

@sam-w
Copy link
Author

sam-w commented Aug 7, 2014

Woooooooow. Actually, according to https://developer.apple.com/support/appstore/ iOS 6 is still 9% of the market, but to be honest that's irrelevant. Plenty of developers still need to support iOS 6, and you're providing a library for developers. Why not provide a graceful fallback where you can?

@sam-w
Copy link
Author

sam-w commented Aug 7, 2014

I'll look into the issue with the text vertical alignment. It will be because I've added the ability to style the placeholder text, and the placeholder text is now different to the normal text.

@ArtSabintsev
Copy link
Owner

Re: iOS 6: I'm of the camp that we shouldn't support stragglers past a certain point, usually 85%. That mindset led me to creating this lib: http://github.com/ArtSabintsev/Harpy.

Supporting older versions passed their prime adds more work and more code to the codebase that overtime will be outdated.

Hell, even Facebook's flagship app only supports iOS 7+ at this point: https://itunes.apple.com/us/app/facebook/id284882215?mt=8

@ArtSabintsev
Copy link
Owner

Re: Vertical alignment - thanks. Good luck!

@ArtSabintsev
Copy link
Owner

Alright, I've reviewed and tested most if not all of the code.

I'll add some of the minor changes tomorrow morning and push it. At that point, merge your fork with my master. Afterwards, we can review the code again, and decide what to do re:

  • animation (==1 conditional)
  • iOS 6 support
  • Vertical alignment visual fixes

Sound good?

@ArtSabintsev
Copy link
Owner

Alright, just pushed a bunch of the changes. Feel free to merge with it and add your attributed changes once more, sans iOS 6 support :)

@ArtSabintsev
Copy link
Owner

Closing this PR as I have already merged a bunch of changes. Feel free to resubmit with the other changes if you want then in here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants