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.

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

Add categories on UILabel, UITextView, and UITextField to accept a BONTextable object #108

Merged
merged 5 commits into from Mar 16, 2016

Conversation

Imperiopolis
Copy link
Collaborator

This allows assigning an id <BONChainable> directly to a UILabel, UITextView, or UITextField in the same way you would assign a font. The BonMot styling assigned to the label/textView/textField will be applied to any future updates to the label/textView/textField's text property.

This works via swizzling the setText: and setAttributedText: methods of these classes to allow applying the BonMot styling when appropriate.

Fixes #68

* Assign a @p BONChainable object to apply to the label text. When a new value is assigned to @p text the chain attributes will be applied.
* If a new value is assigned directly to @p attributedText the @p bonChainable property will be set to @p nil.
*/
@property (nonatomic, copy) id<BONChainable> bonChainable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

#req for properties, please use the order copy, nonatomic to match the rest of the project. (If some place in the rest of the project doesn't match that, we should fix it. It should match the Raizlabs Cocoa Style Gudie)

@ZevEisenberg
Copy link
Collaborator

@jvisenti @ateliercw Here's a PR-in-progress for setting id<BONChainable> on labels, text fields, and text views. In addition to just letting you set things on a one-time basis, it also uses associated objects and swizzles -setText: to update the incoming text with the currently assigned id<BONChainable>. What do you think about that approach? Is it worth keeping in? Is it too risky? Would it make sense to have a flag to disable it, like the -dealloc swizzling in RZDataBinding?

@ZevEisenberg
Copy link
Collaborator

@jvisenti @ateliercw also, do you have opinions on how you would want to handle typingTextAttributes for editable text?

@ZevEisenberg
Copy link
Collaborator

@KingOfBrian @jkaufman you may also have opinions over whether this use case merits swizzling, or whether a subclass would be sufficient. We use swizzling in RZDataBinding to great effect, but it's limited to just -dealloc.

@KingOfBrian
Copy link
Contributor

This is a very interesting use case. Adding more state to UILabel makes me a bit nervous. I'm never clear what happens when I set a font, a text color, text, and an attributed string. Adding a bonChain expands this uncertainty a bit.

It would be good to think through the expected behavior here. It may be that the expected behavior is extracting the font and the color from the bonChain. This functionality could also just assign the color and font to the UILabel when a bonChain is assigned. What happens when there is a bonChain, and a user changes the textColor? It's hard to define a clear contract here (Thanks NSAttributedString!)

The interesting end goal here to be able to assign a bonChain to a label and continue to change just the text property of the label and maintain the style, Correct?

I wonder if this functionality really needs any knowledge of bonChain. It may be that you could just take the attributes out of the current attributedText and create a new attributed string with those same attributes.Honestly I'm not sure, but UILabel may already do this. I always do text + font + color, or attributedText.

@Imperiopolis
Copy link
Collaborator Author

I'm never clear what happens when I set a font, a text color, text, and an attributed string.

So, as I was implementing this I researched this a bit and it has slightly different behavior depending on which class you're using.

For all three, setting text directly clears all manually assigned attributes set through attributedText. Setting attributedText sets text to attributedText.string.

For UILabel and UITextField, textColor and font are applied if the NSAttributedString doesn't have them defined.

For UITextView, textColor and font are ignored when an attributed string is in use.

UITextView also has some default attributes it injects (shadow and color I think, I'd have to double check)

The interesting end goal here to be able to assign a bonChain to a label and continue to change just the text property of the label and maintain the style, Correct?

Correct, that's the desired goal.

I wonder if this functionality really needs any knowledge of bonChain. It may be that you could just take the attributes out of the current attributedText and create a new attributed string with those same attributes.Honestly I'm not sure, but UILabel may already do this. I always do text + font + color, or attributedText.

This is somewhat tricky because some of what BONText does isn't part of the attributes dictionary. Check out the attributedStringLastConcatenant: method on BONText to see some of the other work it does. Also, even if what it styles was strictly attributes it'd be tricky to know when it should preserve attributes on text assignment vs when you're actually trying to get rid of the attributes / remove the attributedString.

It would be good to think through the expected behavior here. It may be that the expected behavior is extracting the font and the color from the bonChain. This functionality could also just assign the color and font to the UILabel when a bonChain is assigned. What happens when there is a bonChain, and a user changes the textColor? It's hard to define a clear contract here (Thanks NSAttributedString!)

I think this is the key question. I implemented in the way that I'd expect it to work, but I have no idea if what I expect matches the general expectations, so opinions very welcome :)

What I expect is that when the bonChainable property is assigned label.bonChainable = chain; label.text = "string" will behave identically to label.attributedText = chain.string("string").attributedString. The things I'm doing to set textColor and font can and should probably go away now that I'm reflecting on this. My initial reasoning there was for UITextField specifically to apply some of the styling to the typingText (and then to the others for consistency across the classes), but this may be better resolved through something like the typingTextAttributes.

@ateliercw
Copy link

I'd advocate putting this into a subspec of bonmot and making it a separate framework for carthage, or going a step further and making it a separate but related library.

It's a useful feature, but it's expanding the scope of bonmot from a attributed string generation library into an attributed string generation library and the UI implementation details.

@jvisenti
Copy link
Contributor

jvisenti commented Mar 7, 2016

I agree with @ateliercw. You could actually pull the BONTextAlignmentConstraint into a UI subspec if you go that way. There could conceivably be more UI-related additions to BonMot, but NSAttributedString generation is really the core framework here, and can stand alone.

@ZevEisenberg
Copy link
Collaborator

@ateliercw +1 on making any UIKit extensions a subspec or separate component thingy, and @jvisenti +1 on moving BONTextAlignmentConstraint in there too. Wondering if that would constitute a "breaking" change re: SemVer?

@Imperiopolis it looks like you are expecting two code paths, one to perform initial styling/setup on UI elements, and a second when updates come in and the text needs to be updated. Without the categories in this PR, you'd have to do a form of dependency injection:

label.attributedText = incomingText + someBONChain

I'm wondering whether the potential code cleanliness is worth the tradeoff of readability/maintainability by having the labels do magic things. Why not structure your code in such a way that label text always passes through a BONChain on its way to the label? For example, if I were using RZDataBinding or a similar binding tool, the only place that a label's text would change would be in a view model property's change handler. If possible, I would prefer to advocate a more composition-based approach, or more lightweight, non-swizzling methods, such as:

// This is a one-way method that sets the attributed based on
// the object passed, but does not store the passed object
- (void)setBONChainable:(id<BONChainable>)bonChainable;

@Imperiopolis
Copy link
Collaborator Author

Moving it to a subspec totally makes sense. I'll work on that change.

@ZevEisenberg:
I want this "magic" in the label because the general use case for me involves setting up a label and then later having the model update the text after doing whatever work/API calls it needs to get the content. Having the model know about the UI details and apply the right chain to the text in this case feels wrong to me and complicates what is generally shared logic in my views/viewControllers.

The way I am treating this in my head is basically an evolution of UIFont and thus to me it feels like it should work similar to the font property on the label where it's essentially set it and forget it.

@ZevEisenberg
Copy link
Collaborator

Having the model know about the UI details and apply the right chain to the text in this case feels wrong to me and complicates what is generally shared logic in my views/viewControllers.

I was figuring that the model would vend data, view model would concatenate that data into a formatted string, and the view would mix that string with a BONChain. Or, the view model vends out a BONChain or NSAttributedString, depending on what you're doing. But, in the spirit of separating content from markup, I can see how it would make sense to have the text and the styling arrive via different channels. Although you're still going to have to set the .attributedString or .bonChainable directly if you're concatenating chains with different styles.

@Imperiopolis
Copy link
Collaborator Author

What you described is spot on with what I meant, I was just oversimplifying. The general path for me is model -> view model -> view, so theoretically I could rely on the view to hang onto the chain and reapply it when it has updated text data. The major problem I see with that approach is I now have to add storage for the chain to every view that behaves this way (basically, every view) so that it can be reapplied later.

But, in the spirit of separating content from markup, I can see how it would make sense to have the text and the styling arrive via different channels. Although you're still going to have to set the .attributedString or .bonChainable directly if you're concatenating chains with different styles.

Yeah, separation of content from markup is my major desire here. There are certainly more complex cases where you have to deal with .attributedString, but my goal is to handle the simple 1:1 style/label relationship. I am thinking of this much like CSS text styling. Yes, you can define content in CSS as you can on a chain, but in general that styling is paired with the class/view and the content is provided on a case by case basis.

Based on this discussion, I'm wondering if my usage of BonMot is different from most. For me, the fact that BonMot generates an NSAttributedString is for the most part purely an implementation detail. The way I'm using it, I treat it as a text style and vend app wide styles (think h1, h2, etc.) from a shared Styles class in the same way as I would previously vend similarly categorized UIFonts from a Fonts class.

@ZevEisenberg
Copy link
Collaborator

Based on this discussion, I'm wondering if my usage of BonMot is different from most.

@Imperiopolis you're, like, the third person I know of who has actually used it in more than a trivial way in a real app. Trailblazer 😄

The way I'm using it, I treat it as a text style and vend app wide styles (think h1, h2, etc.) from a shared Styles class

This is pretty much what I had in mind when I started BonMot. I feel like I've drifted away from that, since I haven't had the chance to use it extensively in an app myself (hoping to change that soon), but that's spot-on for how it was intended to be used.

I still think I originally had kind of a "composition" model in mind, where the way you would always set the text would be to grab the appropriate style, mix your string into it, and pass the resulting attributed string on to the view layer. That's effectively what you're doing, just shifting the responsibilities a little bit.

I wonder whether there's a simple compromise, like making methods like -setTextAndMixItWithYourChain:(NSString *)text instead of swizzling -setText:, so it's clear that there's no magic, and all you have to do is call the other method. Except then it's one more thing to have to remember, so meh ¯_(ツ)_/¯

@ZevEisenberg
Copy link
Collaborator

One more thing, just to stir the pot: I know I suggested the .bonChainable property name, but I wonder if it gets too into the implementation detail weeds? Do you currently need to know what <BONChainable> is to use BonMot? Should you?

@Imperiopolis
Copy link
Collaborator Author

One more thing, just to stir the pot: I know I suggested the .bonChainable property name, but I wonder if it gets too into the implementation detail weeds? Do you currently need to know what is to use BonMot? Should you?

The .bonChainable property name is definitely weird to me but I'm not sure the best work around without having two properties for BONChain and BONText. For what it's worth, in my use case I've never used BONText directly and so having a chain property alone would be sufficient.

I wonder whether there's a simple compromise, like making methods like -setTextAndMixItWithYourChain:(NSString *)text instead of swizzling -setText:, so it's clear that there's no magic, and all you have to do is call the other method. Except then it's one more thing to have to remember, so meh ¯_(ツ)_/¯

Sure, I'm good with that approach as it accomplishes what I want without potentially confusing what setText: does. If that's the consensus I can move to that. To handle the "one more thing to remember" issue, we could still swizzle setText: in debug mode and trigger a log when you call it directly while a chain is defined.

@ZevEisenberg
Copy link
Collaborator

It occurs to me that BONChain should have an initializer that takes a BONText probably. Anyway, looking forward to @KingOfBrian or @ateliercw's thoughts on this latest comment. Sounds like we're heading in the right direction. I was still not thinking of using associated objects to add a stored chain to a label, although I'm much more agreeable to doing it if we're not also swizzling in production. It still rubs me the wrong way a little as an instance of storing state in the UI, but based on your h1, h2, etc. use case, it's much more like just setting the font on a label, which is at least an established pattern. And now I'm back around to being OK with magic and swizzling -setText:. Going to let it sit for a few hours and see if I lean one way or the other.

Anyone else, please weigh in if you have opinions.

@ZevEisenberg
Copy link
Collaborator

Trying to coalesce people's opinions, both from this thread and via private backchannel communications, so we have them all in one place. Let me know if I've misrepresented anything here.

Here are the choices, and I think everyone is agreed that if we do these, they should go in a special UIKit Extensions subspec:

  1. Null Hypothesis: don’t bother making UILabel/UITextField/UITextView utilities. The only way to use BonMot is to get the .attributedString yourself.
  2. Bare Minimum: category method to set a <BONChainable>, which you have to call every time.
  3. Associated Objects: store the <BONChainable> set from the category method, and then add another category method like -setTextAndApplyStylesFromStoredChainable: (the name should be better, but if we can't think of a good name, that may be an indication that we shouldn't do this).
  4. Swizzle: make -setText: automatically apply the associated BONChainable to the incoming string.

Votes:

  • @Imperiopolis: Associated Objects. On the fence about Swizzle.
  • @jvisenti: if we're going to put this in a subspec anyway, in favor of going "full magic" via Swizzle.
  • @KingOfBrian: Leaning towards the Null Hypothesis. But also possibly interested in the Bare Minimum case, and possibly Associated Objects as well. One further idea: if using associated objects, override the getter to return a default chain reflecting the contents and styling of the label.
  • @ZevEisenberg: Bare Minimum. I like the idea of having only a single point of entry for getting BonMot stuff into a UI element. I'm tempted to stick with the null hypothesis, but I definitely see value in having a utility to make it easier to do this common task. This is especially relevant if we move ahead with Support styling localized strings #69, which concerns styling localized strings, meaning that you would be using BONChain directly much more than plain strings.
  • @ateliercw: leaning towards Bare Minimum or Associated Objects.

By my count, with "leaning" or "on the fence" counted as 0.5, that makes it:

  1. Null Hypothesis: 0.5
  2. Bare Minimum: 2.0
  3. Associated Objects: 2.0
  4. Swizzle: 1.5

CC'ing @mgorbach, our software architect, who may have Opinions. Based on what I've seen here, I'd be fine with Bare Minimum or Associated Objects.

@mgorbach
Copy link

Thinking about this, I think I'm fan of implementing an optional label subclass, in a separate framework, that stores bonChain state, and applies it to text. It could disable external screwing with the attributed string, and take full control of it itself instead.
Yes, subclasses can be annoying, but how often do you need to subclass UILabel for functionality other than this? It would be strictly optional, and easy to move off of if you need better control.
You could also do this for TextView and TextField, but I think the value there is more questionable.
I would do label first, and potentially TextField in a second pass.
Just feel like there is enough magic here that it deserves a real subclass.

@Imperiopolis
Copy link
Collaborator Author

For me, a subclass prevents me from using this easily in some of the areas I would like, such as a cell's titleLabel. I could go the route of having a custom cell base class that uses this new label type, but this potentially gets problematic very quickly.

Could you elaborate on the benefit using a subclass brings here? Right now all I'm really seeing is greater usage complexity versus the Associated Objects strategy. With that strategy you need to explicitly set a chain and use the custom setTextAndApplyChain: – the normal paths remain completely unaffected, but you can still use a stock UILabel.

@ZevEisenberg
Copy link
Collaborator

@Imperiopolis makes a good case that with a subclass, you couldn't use this feature with any class that exposes a label directly. I'm liking Associated Objects more and more. If it ends up being popular, we could consider adding swizzling later so we can get @jvisenti's "full magic".

@Imperiopolis
Copy link
Collaborator Author

I've actually leaned more towards the Swizzle side of the fence as I've been using it in my current app. That "full magic" is really valuable in providing a simple and straightforward API.

That said, it definitely seems like the Associated Objects route is a good middle ground and likely the best route to proceed with for now.

@ZevEisenberg
Copy link
Collaborator

Cool, @Imperiopolis why don't, you go ahead with the Associated Objects approach with your PR. Feel free to open an issue for associated objects, but I think we should give it a little time before adding it to see if it would be helpful to more people. Luckily, you can continue to swizzle in your own app if you want. Enjoy your dog food 😉

@Imperiopolis
Copy link
Collaborator Author

To confirm, we also do want to move this into a subspec along with BONTextAlignmentConstraint? Or should that perhaps be pushed to an independent issue? I'm mostly hesitant to make that change as it may be a breaking change for existing users of BONTextAlignmentConstraint.

@ZevEisenberg
Copy link
Collaborator

👍 on the subspec. I'm OK with that breaking change, since the fix is just to add the subspec. I'll still bump the version to 3.0 because of SemVer.

@ZevEisenberg ZevEisenberg changed the title Add categories on UILabel, UITextView, and UITextField to accept a BONChainable object m Mar 11, 2016
@ZevEisenberg ZevEisenberg changed the title m Add categories on UILabel, UITextView, and UITextField to accept a BONChainable object Mar 11, 2016
@ZevEisenberg
Copy link
Collaborator

^ apparently there's an easy way to accidentally edit the name of a PR 😳

@ZevEisenberg
Copy link
Collaborator

There appear to be merge conflicts. Are they just related to the CocoaPods-generated Xcode project?

@@ -1,4 +0,0 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

#? any idea why this was deleted?

@Imperiopolis Imperiopolis force-pushed the nt-utilities branch 2 times, most recently from ab7d080 to 79ca4ab Compare March 12, 2016 02:42
@ZevEisenberg
Copy link
Collaborator

For anyone following along at home, @Imperiopolis and I have been discussing naming things, The Hardest Problem In Computer Science™. We've settled on renaming <BONChainable> to <BONTextable> and the textAndApplyChainable "property" to a real property called bonString.

{
UITextView *textView = UITextView.new;
textView.textAndApplyChainable = @"Hello, world!";
textView.bonChainable = BONChain.new.font([UIFont systemFontOfSize:16]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

#? Since a BONTextable (as it's soon going to be called) contains its own string, it's weird to me that it doesn't overwrite the text in the label. What do you think, @Imperiopolis?

//
//

#import "BonMot.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

#pp #import <BonMot/BonMot.h> - I'm not sure why; I think it's because at one point, the <> import got you autocomplete and the "" one didn't. Now I think I prefer it because it explicitly makes it look like it's a library/framework, and it's scoped, not global.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this is part of the BonMot library I think it should remain #import "BonMot.h". The <...> syntax is intended for imports of files not provided by the project. See this SO thread for more info.

@ZevEisenberg ZevEisenberg changed the title Add categories on UILabel, UITextView, and UITextField to accept a BONChainable object Add categories on UILabel, UITextView, and UITextField to accept a BONTextable object Mar 12, 2016
@@ -112,6 +112,33 @@ NSAttributedString *redBirdString = redBirds.attributedString;
NSAttributedString *blueBirdString = blueBirds.attributedString;
```

## Label, TextView, and TextField Utilities
Copy link
Collaborator

Choose a reason for hiding this comment

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

#nth Change to one of the following:

  1. UILabel, UITextView, and UITextField Utilities
  2. UIKit Utilities

ZevEisenberg added a commit that referenced this pull request Mar 16, 2016
Add categories on UILabel, UITextView, and UITextField to accept a BONTextable object
@ZevEisenberg ZevEisenberg merged commit a55b8bd into Rightpoint:master Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants