[PATCH] tracklist: use lower number unique track IDs

[PATCH] tracklist: use lower number unique track IDs

[Eric Wong]

This is easier for users to read and type; and _might_ help with
race conditions due to fast object recycling from GC.  We'll also be
implementing playist versioning on top of this in the next commits
for MPD protocol compatibility.

Unfortunately this adds an additional 40 bytes of per-track overhead
(on 64-bit systems, its only 20 bytes on 32-bit).  However, we may
be able to save memory in the future by supporting dtas-mlib node
IDs if we integrate dtas-player with DTAS::Mlib.

While we're at it, include a minor speedup for
DTAS::Tracklist#remove_track by using Array#delete_at instead of
relying on Array#compact!  after assignment

This should improve "dtas-tl cat" output readability dramatically.

The state file (~/.dtas/player_state.yml) remains compatible
between dtas-player before and after this change.
---
 lib/dtas/player/client_handler.rb | 15 +++++++-------
 lib/dtas/track.rb                 | 13 ++++++++++++
 lib/dtas/tracklist.rb             | 37 ++++++++++++++++++++++++----------
 test/test_tracklist.rb            | 42 +++++++++++++++++++++++----------------
 4 files changed, 72 insertions(+), 35 deletions(-)
 create mode 100644 lib/dtas/track.rb

diff --git a/lib/dtas/player/client_handler.rb b/lib/dtas/player/client_handler.rb
index fab60f3..0aa264f 100644
--- a/lib/dtas/player/client_handler.rb
+++ b/lib/dtas/player/client_handler.rb
@@ -587,15 +587,16 @@ def dpc_tl(io, msg)
     when "remove"
       track_id = msg.shift or return io.emit("ERR track_id not specified")
       track_id = track_id.to_i
-      @tl.remove_track(track_id) or return io.emit("MISSING")
+      rm = @tl.remove_track(track_id) or return io.emit("MISSING")
+      rm = rm.object_id
 
       # skip if we're removing the currently playing track
       if @current && @current.respond_to?(:infile) &&
-         @current.infile.object_id == track_id
+         @current.infile.object_id == rm
         _tl_skip
       end
       # drop it from the queue, too, in case it just got requeued or paused
-      @queue.delete_if { |t| Array === t && t[0].object_id == track_id }
+      @queue.delete_if { |t| Array === t && t[0].object_id == rm }
       io.emit("OK")
     when "get"
       res = @tl.get_tracks(msg.map!(&:to_i))
@@ -615,11 +616,11 @@ def dpc_tl(io, msg)
         io.emit("MISSING")
       end
     when "current"
-      path = @tl.cur_track
-      io.emit(path ? path : "NONE")
+      track = @tl.cur_track
+      io.emit(track ? track.to_path : "NONE")
     when "current-id"
-      path = @tl.cur_track
-      io.emit(path ? path.object_id.to_s : "NONE")
+      track = @tl.cur_track
+      io.emit(track ? track.track_id.to_s : "NONE")
     when "next"
       _tl_skip
       io.emit("OK")
diff --git a/lib/dtas/track.rb b/lib/dtas/track.rb
new file mode 100644
index 0000000..215ee52
--- /dev/null
+++ b/lib/dtas/track.rb
@@ -0,0 +1,13 @@
+# Copyright (C) 2015 all contributors <dtas-all@nongnu.org>
+# License: GPLv3 or later (https://www.gnu.org/licenses/gpl-3.0.txt)
+require_relative '../dtas'
+
+class DTAS::Track
+  attr_reader :track_id
+  attr_reader :to_path
+
+  def initialize(track_id, path)
+    @track_id = track_id
+    @to_path = path
+  end
+end
diff --git a/lib/dtas/tracklist.rb b/lib/dtas/tracklist.rb
index 63fe4ec..696026a 100644
--- a/lib/dtas/tracklist.rb
+++ b/lib/dtas/tracklist.rb
@@ -2,6 +2,7 @@
 # License: GPLv3 or later (https://www.gnu.org/licenses/gpl-3.0.txt)
 require_relative '../dtas'
 require_relative 'serialize'
+require_relative 'track'
 
 # the a tracklist object for -player
 # this is inspired by the MPRIS 2.0 TrackList spec
@@ -19,7 +20,7 @@ class DTAS::Tracklist # :nodoc:
   def self.load(hash)
     obj = new
     obj.instance_eval do
-      list = hash["list"] and @list.replace(list)
+      list = hash["list"] and @list.replace(list.map! { |s| new_track(s) })
       @pos = hash["pos"] || -1
       @repeat = hash["repeat"] || false
     end
@@ -27,13 +28,27 @@ def self.load(hash)
   end
 
   def to_hsh
-    ivars_to_hash(SIVS).delete_if { |k,v| TL_DEFAULTS[k] == v }
+    h = ivars_to_hash(SIVS)
+    h.delete_if { |k,v| TL_DEFAULTS[k] == v }
+    list = h['list'] and list.map!(&:to_path)
+    h
   end
 
   def initialize
     TL_DEFAULTS.each { |k,v| instance_variable_set("@#{k}", v) }
     @list = []
     @goto_off = @goto_pos = nil
+    @track_nr = 0
+  end
+
+  def new_track(path)
+    n = @track_nr += 1
+
+    # nobody needs a billion tracks in their tracklist, right?
+    # avoid promoting to Bignum on 32-bit
+    @track_nr = n = 1 if n >= 0x3fffffff
+
+    DTAS::Track.new(n, path)
   end
 
   def reset
@@ -49,7 +64,7 @@ def size
   # a few tens of tracks, maybe a hundred at most.
   def _track_id_map
     by_track_id = {}
-    @list.each_with_index { |t,i| by_track_id[t.object_id] = i }
+    @list.each_with_index { |t,i| by_track_id[t.track_id] = i }
     by_track_id
   end
 
@@ -58,12 +73,12 @@ def get_tracks(track_ids)
     track_ids.map do |track_id|
       idx = by_track_id[track_id]
       # dtas-mpris fills in the metadata, we just return a path
-      [ track_id, idx ? @list[idx] : nil ]
+      [ track_id, idx ? @list[idx].to_path : nil ]
     end
   end
 
   def tracks
-    @list.map(&:object_id)
+    @list.map(&:track_id)
   end
 
   def advance_track(repeat_ok = true)
@@ -80,7 +95,7 @@ def advance_track(repeat_ok = true)
     else
       return
     end
-    [ @list[next_pos], next_off ]
+    [ @list[next_pos].to_path, next_off ]
   end
 
   def cur_track
@@ -88,6 +103,7 @@ def cur_track
   end
 
   def add_track(track, after_track_id = nil, set_as_current = false)
+    track = new_track(track)
     if after_track_id
       by_track_id = _track_id_map
       idx = by_track_id[after_track_id] or
@@ -106,27 +122,26 @@ def add_track(track, after_track_id = nil, set_as_current = false)
         @pos += 1 if @pos >= 0
       end
     end
-    track.object_id
+    track.track_id
   end
 
   def remove_track(track_id)
     by_track_id = _track_id_map
     idx = by_track_id.delete(track_id) or return false
-    @list[idx] = nil
-    @list.compact!
+    track = @list.delete_at(idx)
     len = @list.size
     if @pos >= len
       @pos = len == 0 ? TL_DEFAULTS["pos"] : len
     end
     @goto_pos = @goto_pos = nil # TODO: reposition?
-    true
+    track.to_path
   end
 
   def go_to(track_id, offset_hhmmss = nil)
     by_track_id = _track_id_map
     if idx = by_track_id[track_id]
       @goto_off = offset_hhmmss
-      return @list[@goto_pos = idx]
+      return @list[@goto_pos = idx].to_path
     end
     @goto_pos = nil
     # noop if track_id is invalid
diff --git a/test/test_tracklist.rb b/test/test_tracklist.rb
index e3a8d34..33184c6 100644
--- a/test/test_tracklist.rb
+++ b/test/test_tracklist.rb
@@ -3,10 +3,19 @@
 require_relative 'helper'
 require 'dtas/tracklist'
 class TestTracklist < Testcase
+
+  def list_to_path(tl)
+    tl.instance_variable_get(:@list).map(&:to_path)
+  end
+
+  def list_add(tl, ary)
+    ary.reverse_each { |x| tl.add_track(x) }
+  end
+
   def test_tl_add_tracks
     tl = DTAS::Tracklist.new
     tl.add_track("/foo.flac")
-    assert_equal(%w(/foo.flac), tl.instance_variable_get(:@list))
+    assert_equal(%w(/foo.flac), list_to_path(tl))
 
     oids = tl.tracks
     assert_kind_of Array, oids
@@ -14,26 +23,24 @@ def test_tl_add_tracks
     assert_equal [ [ oids[0], "/foo.flac" ] ], tl.get_tracks(oids)
 
     tl.add_track("/bar.flac")
-    assert_equal(%w(/bar.flac /foo.flac), tl.instance_variable_get(:@list))
+    assert_equal(%w(/bar.flac /foo.flac), list_to_path(tl))
 
     tl.add_track("/after.flac", oids[0])
-    assert_equal(%w(/bar.flac /foo.flac /after.flac),
-                 tl.instance_variable_get(:@list))
+    assert_equal(%w(/bar.flac /foo.flac /after.flac), list_to_path(tl))
   end
 
   def test_add_current
     tl = DTAS::Tracklist.new
-    tl.instance_variable_get(:@list).replace(%w(a b c d e f g))
+    list_add(tl, %w(a b c d e f g))
     tl.add_track('/foo.flac', nil, true)
-    assert_equal '/foo.flac', tl.cur_track
+    assert_equal '/foo.flac', tl.cur_track.to_path
   end
 
   def test_advance_track
     tl = DTAS::Tracklist.new
-    tl.instance_variable_get(:@list).replace(%w(a b c d e f g))
-    %w(a b c d e f g).each do |t|
-      assert_equal t, tl.advance_track[0]
-    end
+    ary = %w(a b c d e f g)
+    list_add(tl, ary)
+    ary.each { |t| assert_equal t, tl.advance_track[0] }
     assert_nil tl.advance_track
     tl.repeat = true
     assert_equal 'a', tl.advance_track[0]
@@ -46,7 +53,7 @@ def _build_mapping(tl)
 
   def test_goto
     tl = DTAS::Tracklist.new
-    tl.instance_variable_get(:@list).replace(%w(a b c d e f g))
+    list_add(tl, %w(a b c d e f g))
     mapping = _build_mapping(tl)
     assert_equal 'f', tl.go_to(mapping['f'])
     assert_equal 'f', tl.advance_track[0]
@@ -56,21 +63,22 @@ def test_goto
 
   def test_remove_track
     tl = DTAS::Tracklist.new
-    tl.instance_variable_get(:@list).replace(%w(a b c d e f g))
+    ary = %w(a b c d e f g)
+    list_add(tl, ary)
     mapping = _build_mapping(tl)
-    %w(a b c d e f g).each { |t| assert_kind_of Integer, mapping[t] }
+    ary.each { |t| assert_kind_of Integer, mapping[t] }
 
     tl.remove_track(mapping['a'])
-    assert_equal %w(b c d e f g), tl.instance_variable_get(:@list)
+    assert_equal %w(b c d e f g), list_to_path(tl)
 
     tl.remove_track(mapping['d'])
-    assert_equal %w(b c e f g), tl.instance_variable_get(:@list)
+    assert_equal %w(b c e f g), list_to_path(tl)
 
     tl.remove_track(mapping['g'])
-    assert_equal %w(b c e f), tl.instance_variable_get(:@list)
+    assert_equal %w(b c e f), list_to_path(tl)
 
     # it'll be a while before OIDs require >128 bits, right?
     tl.remove_track(1 << 128)
-    assert_equal %w(b c e f), tl.instance_variable_get(:@list), "no change"
+    assert_equal %w(b c e f), list_to_path(tl), "no change"
   end
 end
-- 
EW

[PATCH 2/1] tracklist: do not mutate @list when serializing

[Eric Wong]

This happens when "dtas-ctl state dump" is invoked manually;
causing "dtas-tl cat" to break afterwards.

Fixes: commit 7b065706d37df9e54c8b3299ce696545c6159fa4
       ("tracklist: use lower number unique track IDs")
---
 lib/dtas/tracklist.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dtas/tracklist.rb b/lib/dtas/tracklist.rb
index 696026a..1750f26 100644
--- a/lib/dtas/tracklist.rb
+++ b/lib/dtas/tracklist.rb
@@ -30,7 +30,7 @@ class DTAS::Tracklist # :nodoc:
   def to_hsh
     h = ivars_to_hash(SIVS)
     h.delete_if { |k,v| TL_DEFAULTS[k] == v }
-    list = h['list'] and list.map!(&:to_path)
+    list = h['list'] and h['list'] = list.map(&:to_path)
     h
   end
 
-- 
EW