Review Board

1.0

Mélodie improvements

Updated 1 year, 8 months ago

Truls Becken Reviewers
EtoileCore
None Etoile trunk (etoile/trunk/Etoile) 
- Cache the play/pause icons rather than calling out to IconKit once per second.
- Detect when a new song is playing by comparing the identities of URLs from MediaKit.
- Use the MediaKit queue size to know which ETMusicFile is current.
- Update the song label when the URL changes (previously on double-click only).
- Keep the MediaKit queue at a couple of songs rather than pushing everything at once.
- Implement the previous button using ETHistoryManager.
- Disable the next/previous buttons when appropriate, and make them appear dimmed (Thanks Quentin)
- Fire the UI update timer after actions that are likely to require a refresh.

Although there's no reason for it to block this commit, I'll be the first to say that I don't think Mélodie should be using a history manager for the songs played. It depends a little, but the more natural approach is probably to use the playlist. This, however, must be seen in a slightly larger context. What should happen when one clicks on a song in the main list? If Mélodie is going to be playlist driven, it should just add the song at the end of the playlist. This also makes things less confusing when one keeps changing the main list through searching.

Also, I think the MusicPlayerController should not be concered with items from EtoileUI, but rather ETMusicFile objects directly. I might look into this a little more since it's a good opportunity for learning about CoreObject.

 

 





  

 















 

  




 

 

  
  
David Chisnall

  
Posted 1 year, 8 months ago (December 15th, 2008, 2:02 p.m.)

 

 

   
Looks good to me, but Eric's the Mélodie maintainer, so wait for him to check it before committing.

   

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Looks good, thanks for cleaning up Melodie :)
Feel free to commit.

I agree that a history manager is the wrong model for the previous/next track buttons, but that can be changed later.
These two navigation systems (next/previous object in ordered list, and back/forward in history) are good to keep in mind since they show up in a lot of places.

I don't like the default amarok 1.x behaviour where (it seemed to me) you always had to be creating a playlist. I think, of the actions avialable on music files, "play" is a better candidate for using as the double-click action than "add to a default playlist." I think the basic model of itunes is better, where playing things in their normal order is the main focus, and creating playlists is a secondary activity (but I'm biased because I don't create playlists too often.) It's an interesting topic that needs more discussion and thought.

Regarding MusicPlayerController being concerned with only ETMusicFiles, I agree, that is cleaner - it might be easier to do that with a cursor feature for EtoileUI/CO

    
    
    
   
    2. 

   


  




   



    

     
     
    

    

     
Smalltalk code should use +new, not +alloc/-init, unless you want to create memory leaks.

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Yes, actually I thought about that and meant to ask you about it.
In this case it's simple, and I'll change it to +new.

But what about other cases where there is no corresponding +new method? One would have to either call release after assigning to the ivar, or autorelease before the assign, right?

Should the threaded object case stick in an autorelease like this?:

player := MKMusicPlayer alloc initWithDefaultDevice autorelease inNewThread.
    
    
    
   
    2.