Change behavior of == to operate on song hashes

[DenialAdams]

for callbacks. From the issue thread:

I did some digging and I think I found a rather smooth solution to this issue! The thing is, the MPD song details change when the radio track changes but comparing the songs with == returns true - even though the object has changed! Consider this:

head :029 > test
 => #<MPD::Song:0x000000016c9750 @data={:name=>"RPGN Radio", :pos=>0, :id=>51}, @time=nil, @file="http://stream.rpgamers.net:8000/rpgn", @title="Infinite Undiscovery - Pure Alabaster", @artist=nil, @album=nil, @albumartist=nil>
head :030 > test2
 => #<MPD::Song:0x0000000168e1c8 @data={:name=>"RPGN Radio", :pos=>0, :id=>51}, @time=nil, @file="http://stream.rpgamers.net:8000/rpgn", @title="Okami - Shinshuu Plains I & II", @artist=nil, @album=nil, @albumartist=nil>
head :033 > test === test2
 => true 
head :034 > test == test2
 => true 
head :035 > test.eql? test2
 => false 
head :036 > test.equal? test2
 => false 

So, if we used .eql? or .equal? (I am not exactly sure which one is better) instead of == at https://github.com/archSeer/ruby-mpd/blob/master/lib/ruby-mpd.rb#L228 then the MPD song callback would trigger when the track in a stream changes. I think this change makes a lot of sense. My only concern is that it might cause unexpected behavior with other callbacks (although I doubt it) but we should test and make sure everything still works as expected.

Also, I checked the refactored version in #32 and it seems that it would need to be changed in the same way.

Thanks, let me know what you think!

[archseer]

Problem here is that I think the default implementation of .eql? will compare the actual object instances. The gem currently generates a fresh new Song instance every time you call #status, so even if the song hasn't changed, .eql? will fail, meaning we're going to trigger the callback on every loop iteration.

[archseer]

The solution instead is to edit the == method on the Song model to also include title:

  def ==(another)
+    self.class == another.class && self.file == another.file
-    self.class == another.class && self.file == another.file && self.title == another.title
  end

[DenialAdams]

Ah, got it. Sorry for missing that, I got a bit too excited... What if just the artist changes, for example? Or some other piece of metadata? Although it seems extremely extremely unlikely the title would stay the same but the artist changes

[archseer]

Good point! Hmm, maybe we could implement the .eql? method then, which casts both songs to_h and compares the hashes?

[DenialAdams]

That solution seems perfect! Great idea.

[DenialAdams]

Would you like to implement that or should I give it a go?

[archseer]

You can update this PR and I'll get it merged and released to rubygems

[DenialAdams]

How does that look? Hope it's ok, I'm not very used to contributing to other code bases >.<

[attilagyorffy]

Just my $0.02: I'd rewrite the implementation of the == method itself and alias eql? like the following:

def ==(another)
  to_h == another.to_h
end

alias :eql? :==

[DenialAdams]

Ah! That seems better, as long as nothing is relying on the behavior of the current == (same file)

[attilagyorffy]

Also, I've had a similar solution in my local branch that I was planning on pushing up at some point but I have to admit that I find the suggestion above much more elegant and idiomatic. It's great to see ruby-mpd improving and having other peoples' continued support. \o/

[DenialAdams]

Ah, thank you very much ^^

[archseer]

@AStrangeEnigma cheers! Can you please squash these commits, then I'll merge?

[DenialAdams]

There you go! Thanks, I was not aware about squashing commits prior to this :)

[archseer]

Released as v0.3.3 :)