From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS22989 208.118.235.0/24 X-Spam-Status: No, score=-2.2 required=3.0 tests=AWL,BAYES_00,URIBL_BLOCKED shortcircuit=no autolearn=unavailable version=3.3.2 X-Original-To: dtas-all@80x24.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 12E432055C for ; Sat, 5 Dec 2015 02:32:14 +0000 (UTC) Received: from localhost ([::1]:44273 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a52dx-0000Ll-9q for dtas-all@80x24.org; Fri, 04 Dec 2015 21:32:13 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a52du-0000I7-58 for dtas-all@nongnu.org; Fri, 04 Dec 2015 21:32:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a52dp-0004jm-TC for dtas-all@nongnu.org; Fri, 04 Dec 2015 21:32:10 -0500 Received: from dcvr.yhbt.net ([64.71.152.64]:36322) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a52dp-0004jh-LE for dtas-all@nongnu.org; Fri, 04 Dec 2015 21:32:05 -0500 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 9F8602055A for ; Sat, 5 Dec 2015 02:32:04 +0000 (UTC) From: Eric Wong To: Subject: [PATCH] tracklist: use lower number unique track IDs Date: Sat, 5 Dec 2015 02:32:19 +0000 Message-Id: <20151205023219.19364-1-e@80x24.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 64.71.152.64 X-BeenThere: dtas-all@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dtas-all-bounces+dtas-all=80x24.org@nongnu.org Sender: dtas-all-bounces+dtas-all=80x24.org@nongnu.org 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 +# 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