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

Sound Effect Submenu #10

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

chrisballinger
Copy link

@chrisballinger chrisballinger commented Feb 3, 2018

screen shot 2018-02-03 at 1 32 45 pm

Implements feature discussed in #9:

  • Splits macOS system sound into separate files for up/down events
  • Allows user to choose a different sound for up/down events (or none)
  • Customizable volume
  • Live preview when changing sound selection or volume
  • Allows user to add/remove their own sound files in Finder. The 00550 wcls, etc listings aren't there normally, just the two defaults, but I wanted to show how custom sound files would be displayed.
  • Localizable interface strings

Questions:

And... it's very common comment in open source world though, it would be really appreciate if you can use same coding format as the one used on master.

Can you elaborate what you mean by this comment?

I'm sure there are other things that could be cleaned up (like deleting the old system sound player), but I wanted to open this PR for your feedback, as I feel like it's now feature complete. I tried to leave edits to upstream as minimal as possible, and keep all of the new functionality as self-contained as possible.

@niw
Copy link
Owner

niw commented Feb 9, 2018

Thank you for the pull request!!!

And sorry for the late reply. Before taking a code review, I want to answer and comments for the previous comments in #9.

  1. It would be better to separate default sounds in terms of UI also may be implementation (how to store the generated separated sound files.)

    I’m suggesting to have a separate menu item that allows users to select Default, Custom, (and probably None as well) as a primary option, then in Custom, allows them to select custom sound files per events, because this customization menu is a bit complicated and for advanced usage.
    In terms of user interface design, that would be better to be in the next level to not surprise users.

    Also, I think Default sounds a part of application so want to hide it from the users so that it can be always under managed. Because of that, I think it would be better to store default sound files should in somewhere temporary or not user customizable area like ~/Library/Application Support, but ~/Library/Caches or in memory.

  2. Code format style

    I don’t have written documents yet there are a few formatting rules I’m using in the code base as a standardize code format. TL; DR, next style.

     @import Foundation;
    
     NS_ASSUME_NONNULL_BEGIN
    
     @interface Cat : NSObject
    
     @property (nonatomic, copy, nullable) NSString *meowing;
    
     @end
    
     @implementation Cat
    
     - (void)meow
     {
         if (self.meowing) {
             NSLog(@"%@", self.meowing);
         }
     }
    
     @end
    
     NS_ASSUME_NONNULL_END
    
    1. Indent with 4 spaces, no tabs.
    2. Put { and } for methods in a new line, not following the line, however, not for if or other statements.
    3. Don’t align arguments by :, instead, put it in a single line.
    4. Give NS_ASSUME_NONNULL_BEGIN and NS_ASSUME_NONNULL_END for both headers and implementations to make nonnull default, then give only nullable or _Nullable for nullable properties, arguments.

@chrisballinger
Copy link
Author

Thanks! I think that's all pretty reasonable. I'll throw together a UI mockup and send a screenshot to make sure I understand what you're thinking before I start my revisions.

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.

2 participants