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

Added JSQAudioMediaItem class #1495

Merged
merged 10 commits into from Apr 5, 2016
Merged

Conversation

eliburke
Copy link
Collaborator

@eliburke eliburke commented Mar 9, 2016

Implements #1049
Added a test case for the JSAAudioMediaItem class
Added a sample audio file to the demo project
Added a sample audio message to the demo project
Added a UIImage category to return the default pause image
Added the JSQAudioMediaItem header to the main header file
Added pause image (Creative Commons License) from NounProject:
https://thenounproject.com/term/pause/183517/

Added a test case for the JSAAudioMediaItem class
Added a sample audio file to the demo project
Added a sample audio message to the demo project
Added a UIImage category to return the default pause image
Added the JSQAudioMediaItem header to the main header file
Added pause image (Creative Commons License) from NounProject:
https://thenounproject.com/term/pause/183517/
@jessesquires
Copy link
Owner

@eliburke awesome! 🎉

Glanced through this. First impressions: 👍

I'll have a bit more feedback soon, within the next couple days. 😄

@jessesquires
Copy link
Owner

Also -- is this you? 😄

@jessesquires jessesquires self-assigned this Mar 10, 2016
@jessesquires jessesquires added this to the 7.2.1 milestone Mar 10, 2016
@eliburke
Copy link
Collaborator Author

Yep, that's me. As you can see from the timeline I don't spend much time on twitter. 😜

I haven't payed a lot of attention to how you make the sausage, but if you need me to fix bugs like the Travis CI warning, or tweak the API / property names, or submit to release_8.0, just let me know.

Big-Pi pushed a commit to iShawnWang/PiChat that referenced this pull request Mar 10, 2016
…1495 有人 pull requset ~ :) 完成录音基本功能,发送,明天添加个录音时的提示 HUD 就 ok
@jessesquires
Copy link
Owner

Hey @eliburke -- sorry for the delay here. I haven't forgotten. Will review soon.

…back and

options to duck other audio.
added a delegate to notify when the sound category is changed
cleaned up some warnings and removed an unused property
@eliburke
Copy link
Collaborator Author

No worries.
I've updated my PR to fix the warnings and remove an unused property. I added a new property to allow users to control the playback audio category, and a delegate method to get notification if the audio category changes. My reasoning for this design is:

  1. developers who don't care what sound category is set can ignore it. JSQAudioMediaItem will set the category to Playback and the options to Duck, Speaker, Bluetooth. Sound will play through the mute switch and to the selected outputs.
  2. developers who need different sound options can set them
  3. anyone who needs to restore the sound category to a particular setting can do so when they clean up the view controller. I didn't want to try and restore the category within JSQAudioMediaItem because it is possible to have multiple audio streams playing at once, and without some sort of singleton to manage the setting, things would likely go haywire.

If you think it's necessary I could add additional methods for starting and ending playback.


@class JSQAudioMediaItem;
@protocol JSQAudioMediaItemDelegate <NSObject>
@optional
Copy link
Owner

Choose a reason for hiding this comment

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

let's make these non-optional

@jessesquires
Copy link
Owner

Thanks @eliburke ! Finally left some comments. Mostly organization stuff.

One other concern:

Can we simplify by only storing the NSData or the NSURL and provide a computed property for the one we don't keep?

Actually, I don't think we can go NSData -> NSURL, but only NSURL -> NSData.

This will help simplify things a lot if we only have to worry about NSData internally.

@JOMEN5
Copy link

JOMEN5 commented Mar 22, 2016

looking forward to your guys update for JSQAudioMediaItem,I can't wait to use it.

@eliburke
Copy link
Collaborator Author

@JOMEN5 I'm on vacation with the family right now. Hard to find time for coding between kids and rum punch. I'll try to get an updated PR in the next couple of days.

@eliburke
Copy link
Collaborator Author

@jessesquires I've made the majority of your changes:

  • Collapsed the protocol to one non-optional method
  • Added a view config class
  • added a designated init and various convenience init methods
  • updated the timestamp format to match iMessages
  • updated various comments and pragmas
  • updated the fade in/out timer
  • added timer manipulation methods
  • removed the internal audio URL and isReadyToPlay properties, replaced with a convenience setter that calls NSData:dataWithContentsOfURL

The one change I did not make was moving the mediaView logic out into a separate class. To be frank, I don't see any value, just extra complexity. If you are thinking that this re-design would allow someone to provide their own view class that plugs into the audio media item, all of the avAudioPlayer controls and the timer would have would have to be shimmed into the new class with yet another protocol. Or, if you put the avAudioPlayer and timer into the new view class, the audio media item class becomes an empty container.

If someone REALLY wants to revamp the look of the audio player beyond the viewConfig settings, I think it would be far simpler to copy and modify the base implementation to suit.

@JOMEN5
Copy link

JOMEN5 commented Apr 4, 2016

@eliburke Thanks for your contribution! I'm here also remain @jessesquires update this PR when you have free time. Best wishes.


@end

@interface JSQAudioMediaViewConfiguration : NSObject
Copy link
Owner

Choose a reason for hiding this comment

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

let's put this new class in its own file 😄

@jessesquires
Copy link
Owner

Thanks so much @eliburke !

Left a couple more really minor comments. Then I think this is good to go. 👍

@codecov-io
Copy link

Current coverage is 66.12%

Merging #1495 into develop will increase coverage by +0.17% as of d65843f

@@            develop   #1495   diff @@
=======================================
  Files            59      63     +4
  Stmts          2109    2294   +185
  Branches          0       0       
  Methods                           
=======================================
+ Hit            1391    1517   +126
  Partial           0       0       
- Missed          718     777    +59

Review entire Coverage Diff as of d65843f

Powered by Codecov. Updated on successful CI builds.

@JOMEN5
Copy link

JOMEN5 commented Apr 4, 2016

Thanks @jessesquires and have you finish the task now? How can I get the latest version,just use pod update or pod 'JSQMessagesViewController', :git => 'https://github.com/jessesquires/JSQMessagesViewController.git', :branch => 'develop'?Looking forward to your reply.

eburke added 3 commits April 4, 2016 14:38
Added boilerplate copyright code to all new files
Added { } curlies where missing
Updated properties to follow standard coding style
Merge /w develop
@jessesquires
Copy link
Owner

@eliburke let me know when those last couple fixes are pushed and i'll merge.

(or, if travis behaves, merge yourself 😄 )

eliburke and others added 3 commits April 5, 2016 09:08
Added a test case for the JSAAudioMediaItem class
Added a sample audio file to the demo project
Added a sample audio message to the demo project
Added a UIImage category to return the default pause image
Added the JSQAudioMediaItem header to the main header file
Added pause image (Creative Commons License) from NounProject:
https://thenounproject.com/term/pause/183517/
Added a delegate to notify when the sound category is changed
Change the sound category to AVAudioSessionCategoryPlayback and
options to duck other audio.
Moved view properties to JSQAudioMediaViewAttributes
Moved button image init to JSQAudioMediaViewAttributes
Added convenience setter for url->nsdata
@eliburke eliburke merged commit f656a7b into jessesquires:develop Apr 5, 2016
@eliburke eliburke deleted the audioMedia branch April 5, 2016 15:39
@jessesquires
Copy link
Owner

🎉

MacMeDan pushed a commit to MacMeDan/JSQMessagesViewController that referenced this pull request May 18, 2016
@eliburke eliburke mentioned this pull request May 23, 2016
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants