Skip to content
This repository has been archived by the owner on Aug 14, 2019. It is now read-only.

Update demo project for AudioMediaItem #1531

Closed
jessesquires opened this issue Apr 6, 2016 · 6 comments
Closed

Update demo project for AudioMediaItem #1531

jessesquires opened this issue Apr 6, 2016 · 6 comments
Assignees
Milestone

Comments

@jessesquires
Copy link
Owner

Follow up from #1495

We added a -addAudioMediaMessage method, 👍

Let's update the actionsheet to simulate sending an audio item.

Follow existing pattern for sending photo/location/video.

  1. Tap 📎 icon
  2. Tap "Send audio"
@jessesquires
Copy link
Owner Author

@eliburke — Be sure to pull the latest. I pushed a couple minor commits to cleanup some of the audio stuff.

Mostly just formatting, docs, and nullability.

See d2e4a06 and 6873264

😄 A few small notes:

  • Always prefer self.property except inside init and dealloc
  • I made JSQAudioMediaViewAttributes immutable and added a designated init. We should always aim for this. Unfortunately, the MediaItem subclasses have to be mutable.
  • NS_ASSUME_NONNULL_* macros make nullability way simpler. These aren't used elsewhere in library yet, unfortunately, because their addition would be a breaking change for existing Swift code 😢 We'll add them in 8.0 everywhere.

@eliburke
Copy link
Collaborator

@jessesquires Sorry I missed this task in the notification spam last week.

Re your change to the init methods in d2e4a6 I feel like that makes the default behavior more difficult to override. If someone simply wants to tweak the font or color, they have to pull in all that other stuff as well. And if we want to add more properties, it breaks the API. Why is it better?

@jessesquires
Copy link
Owner Author

No worries! 👍

If someone simply wants to tweak the font or color, they have to pull in all that other stuff as well.

Great point. Ok -- let's remove readonly from properties, and override the setters to NSParameterAssert(value != nil).

And if we want to add more properties, it breaks the API. Why is it better?

  • Generally, I think preferring immutability is best. It greatly simplifies the programming and threading models. But, you make a good point that it negatively impacts usability here.
  • API breakage: yes, this is always a concern. Generally, the plus side here (I think) is that this kind of pattern really forces you to think about new APIs before shipping, and forces you to really consider any breaking changes.

@jessesquires
Copy link
Owner Author

fixed in #1540

@eliburke
Copy link
Collaborator

After removing readonly, that cool new macro NS_ASSUME_NONNULL_BEGIN triggers nil warnings. If you feel strongly I'll add the asserts, but IMO it's unneeded bloat.

screen shot 2016-04-12 at 9 44 12 pm

@jessesquires
Copy link
Owner Author

Can audioCategory be nil?

If so, we can make it @property (nonatomic, nullable) ... and then what you have above is totally fine.

Otherwise, all non-nullable properties should assert

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

No branches or pull requests

2 participants