search

TheStalwart / Theremin

Undead Mac OS X MPD Client.
169 stars 25 forks source link

Pre-filled Playlist field #6

Closed phpwutz closed 11 years ago

phpwutz commented 11 years ago

Hello, I did a small patch on the PlaylistFilesController which stores the last loaded playlist and prefills this value in the "Save Playlist as" textfield. If since application start no playlist was loaded, it will fill an empty string. I'd be glad if you can pull that little convenience. Please note I'm not an experienced Cocoa developer and feel free to give me feedback about bad code. best regards Lukas

TheStalwart commented 11 years ago

Code is bad indeed, i won't merge this pull request. But i can give you some feedback for sure.

  1. There are three buttons in lower left corner. You've assigned tooltips to only two of them. That's inconsistent. Inconsistency is bad.
  2. You've set library button tooltip to "Your Music Library". That's bad. Like, Windows 98 bad. What if library isn't mine? That would be misleading. Even if library is mine, can i have volume slider too? Or maybe "My remaining time"? Actually, "My remaining time" would be quite frightening.
  3. You've set playlist pane button tooltip to "Show playlists", but didn't implement label swapping to "Hide playlists" for when playlist pane is already open. Play around iTunes "View" menu to see what i mean.
  4. You've introduced "currentPlaylist" instance variable to PlayListFilesController class. There's a reason why Apple discourages use of instance variables lately - because such instance variables aren't managed by memory management syntactic sugar. On line 158 of PlayListFilesController.m you assign currentPlaylist an address of some NSString without incrementing a reference counter. After a while, that string is released, and your *currentPlaylist pointer becomes a tripping point for "Save playlist" dialog causing an application to crash with segmentation fault. To learn about Objective-C memory management, both manual (as used in this project) and newer, Automatic Reference Counting, read this Apple documentation - https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/MemoryMgmt/Articles/MemoryMgmt.html

This codebase is too old to learn from. If you want to get into Objective-C, i'd recommend iOS SDK. Writing iPhone apps is much more fun. Good luck!

phpwutz commented 11 years ago

Hi, So few changes and yet so many things done wrong -> phew… I guess I need a refresher course in Human Interface Guidelines.

This codebase is too old to learn from. If you want to get into Objective-C, i'd recommend iOS SDK. Writing iPhone apps is much more fun. Good luck!

Thanks for the tip, but as I prefer to use Android mobile phones this will probably not happen ;-)

all the best and thx for the feedback

Am 23.05.2013 um 10:14 schrieb TheStalwart notifications@github.com:

Code is bad indeed, i won't merge this pull request. But i can give you some feedback for sure.

There are three buttons in lower left corner. You've assigned tooltips to only two of them. That's inconsistent. Inconsistency is bad. You've set library button tooltip to "Your Music Library". That's bad. Like, Windows 98 bad. What if library isn't mine? That would be misleading. Even if library is mine, can i have volume slider too? Or maybe "My remaining time"? Actually, "My remaining time" would be quite frightening. You've set playlist pane button tooltip to "Show playlists", but didn't implement label swapping to "Hide playlists" for when playlist pane is already open. Play around iTunes "View" menu to see what i mean. You've introduced "currentPlaylist" instance variable to PlayListFilesController class. There's a reason why Apple discourages use of instance variables lately - because such instance variables aren't managed by memory management syntactic sugar. On line 158 of PlayListFilesController.m you assign currentPlaylist an address of some NSString without incrementing a reference counter. After a while, that string is released, and your *currentPlaylist pointer becomes a tripping point for "Save playlist" dialog causing an application to crash with segmentation fault. To learn about Objective-C memory management, both manual (as used in this project) and newer, Automatic Reference Counting, read this Apple documentation - https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/MemoryMgmt/Articles/MemoryMgmt.html This codebase is too old to learn from. If you want to get into Objective-C, i'd recommend iOS SDK. Writing iPhone apps is much more fun. Good luck!

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

TheStalwart commented 11 years ago

Android SDK, Windows Phone SDK - whatever. Mobile apps are fun.