Added ability to play sound when message is displayed

[forgot]

Allows users to set sounds for use when displaying messages.

Users can set a single sound for use with all messages by calling:
+ (void)setNotificationSoundWithName:andExtension:

...or set a sound for use with a specific TSMessageType by calling:
+ (void)setSoundWithName:extension:forNotificationType:

If no sound is set for a particular TSMessageType, but is set with
+(void)setNotificationSoundWithName:andExtension:, the latter will be played as a fallback. If there is nothing to fallback to, then no sound is played.

I look forward to hearing your thoughts and opinions on this.

[KrauseFx]

Thanks for this pull request.
@mRs- @forgot I don't think the TSMessages library should required AVFoundation. Instead how about putting the sound support in a sub spec? What do you think?

[forgot]

@KrauseFx Sounds fine to me. I'm not all that familiar with podspec files, so I'll have to look into the documentation before I make the changes.

[forgot]

@KrauseFx Since I'm new to this, mind checking this before I make a commit?

Within the .podspec I'd go from:

.......  
  s.requires_arc = true
  s.framework    = 'AVFoundation'
  s.dependency 'HexColors'
end

to

........
  s.requires_arc = true
  s.dependency 'HexColors'

    s.subspec 'Sounds' do |sounds|
    sounds.framework    = 'AVFoundation'
  end

end

Does that look correct?

[KrauseFx]

@forgot Thanks, yep, the pod spec looks fine. But I guess the code also has to be adapted: you can not include or call AVFoundation related code.

[forgot]

@KrauseFx
facepalm
I'm self-taught, and sometimes suffer for it.

I'll have to think about how to break it out from the main classes, and still get it to play when a message is displayed. If anyone has any thoughts/ideas on this, I'm all ears.

[KrauseFx]

Yes, I guess it's not that easy. Maybe it would be interesting to add a delegate protocol for the messages, which can also be used by developers to do something when the message was dismissed?

[forgot]

I created a category on TSMessage, and moved the methods there, then added a __has_include macro to TSMessageView+Private like so:

#if defined(__has_include)
#if __has_include("TSMessage+Sounds.h")
#include "TSMessage+Sounds.h"
#define kCanPlaySounds YES
#else
#define kCanPlaySounds NO
#endif
#endif

#define kTSMessagePlaySound @"TSMessagePlaySound"

I also updated the .podspec so that everything from TSMessages is set as a default_subspec, and thus always included, then added another subspec for Sounds. This should allow for someone to add the TSMessages pod to get the regular implementation, or TSMessages/Sounds to get the regular implementation plus the new category and associated AVFoundation.framework.

To get the sounds to play when the message is animated, I'm sending a notification that only fires if kCanPlaySounds = YES, and using a TSMessageSoundPlayer object hitchhiking inside the categories .h/.m files.

Let me know what you think about the revisions. If you still want to do it via a protocol, thats totally fine, I just really wanted to see if I could wrap this up into one simple package that allows users to add sounds quickly without having to add a lot of code. I figured doing it this way made it convenient for the end user. That, and I really just wanted to see if I could accomplish it.

[KrauseFx]

Wow, that looks amazing. Thanks for your great contribution. I wonder why there was never a request for sound for messages - it's kind of obvious.

[forgot]

No problem, I'm really glad you like it. I always wondered that about sounds as well, so I jumped at the chance. Thanks for the opportunity for me to learn something new!

Let me know if you think it could be tweaked any.

[forgot]

@KrauseFx Is there anyone else that should review this? @dennisreimann perhaps? I know he's done quite a bit of work on this branch getting it ready for 1.0. I just want to make sure its solid before the changes get merged, if thats what ends up happening.

[dennisreimann]

@forgot yeah, I'll have a look. Sorry for not chiming in earlier, I'm just pretty busy right now. Thanks for your contribution!

[forgot]

@dennisreimann Its no problem, I know everyone has other work to do. I'm a firm believer in constructive feedback so, whenever you get the free time, let me know if it could be better and I'll do my best to make it so.

[dennisreimann]

Was it a conscious decision to load the sounds beforehand and not lazily when they are needed for the first time?

I'm a little concerned about the five variables needed for storing the AVAudioPlayers - maybe there's a better way to store and handle the sound references.

I think this should also be wired up with the custom design JSON.

@KrauseFx @mRs- reading up on the conversation I think that adding delegation methods for events like willDisplayNotification: and didDismissNotification: might be a good way to provide APIs to hook into TSMessages, so they could implement things like playing sounds themselves. I don't want to cancel this pull request as I think playing sounds might be a good addition, but I'm a bit worried about the weight of the added code.

[mRs-]

Imho we should provide solid delegate solutions for providing sounds.

Maybe an example in an subspec.

Delegates for some actions would be really nice for customizing.

Von meinem iPhone gesendet

Am 16.05.2014 um 23:24 schrieb Dennis Reimann notifications@github.com:

Was it a conscious decision to load the sounds beforehand and not lazily when they are needed for the first time?

I'm a little concerned about the five variables needed for storing the AVAudioPlayers - maybe there's a better way to store and handle the sound references.

I think this should also be wired up with the custom design JSON.

@KrauseFx @mRs- reading up on the conversation I think that adding delegation methods for events like willDisplayNotification: and didDismissNotification: might be a good way to provide APIs to hook into TSMessages, so they could implement things like playing sounds themselves. I don't want to cancel this pull request as I think playing sounds might be a good addition, but I'm a bit worried about the weight of the added code.

—
Reply to this email directly or view it on GitHub.

[forgot]

@dennisreimann No, it wasn't a conscious decision, just my newbness showing. I'll change it to where they're loaded lazily.

You mention the weight of the code. Is the included framework the source of concern, or the overall implementation?

While my initial intention was to have this all wrapped up in a nice package, I'm more than happy to use protocol methods like you mentioned (willDisplayNotification: and didDismissNotification:). Having those methods handy would open up several doors. I'll let any further discussion from the collaborators be my guide.

[forgot]

@dennisreimann @mRs- @KrauseFx

Did some redesign. I removed the TSMessage (Sounds) category (and all the messy static variables that went along with it), and added a TSMessageDelegate protocol with optional methods for willDisplayNotification:, didDisplayNotification:, and didDismissNotification:, then extended it as TSMessageSoundDelegate and the optional soundForNotificationType method.

It still needs the AVFoundation framework, but that will only be added if the /Sounds module is included. My only other concern is that AVAudioPlayer leaks a little (see here for some pretty good discussion on it).

Pick it apart, and let me know what you think. :)

[dennisreimann]

Awesome, I like this way more than the previous state - the delegation methods offer good hooks and the sound option is a really nice example of how the delegation can be used 👍

I haven't had the time to check this out in Xcode and go it through in detail, but in general I think this fits good into where we are going with 1.0, so what do you think @KrauseFx and @mRs- ?

[mRs-]

Awesome Pull Request imho 👍

[forgot]

I'm very glad y'all like it! This is only the second time I've made a contribution to a project like this, and the constructive feedback offered is an incredible help. Massive kudos to you all.

I know everyone hasn't had a chance to fully review this yet, so I may add in one more commit with some documentation comments if that would be helpful. Obviously, if any changes need to be made I'll add those too.

[mRs-]

At the Moment we have no public place to discuss the direction of 1.0.

The last status we were send of is a overall revamp of the code from @dennisreimann but at the moment the time of dennis is very limited. Maybe we should start creating a roadmap for 1.0 to create a direction in were we are heading with 1.0, especially with the announce of swift.

[dennisreimann]

I think there's a 1.0 milestone created by @KrauseFx where Felix listed all the issues that should be worked on before we declare it 1.0 - let's just open a 1.0 issue where we discuss further.

[forgot]

I found the 1.0 milestone after I submitted that comment, and removed that question since I figured I'd found it, lol. A roadmap or issue would be awesome. This project has been super helpful in my own app, and I'd love knowing if there are ways I can contribute further.

[KrauseFx]

Yes, there is a milestone for that, containing a few issues: https://github.com/toursprung/TSMessages/issues/milestones/1/edit

[forgot]

Anything else y'all would like to see added/changed before I quit messing with it? A Swift version perhaps? :) (That may take a little time though, being that I have to actually learn Swift first.)