I have not had a chance to look at this, so filing this so it doesn't get lost.


From an email thread with Triode:

So I don't think you should see those problem cases again as I've fixed the bug in the server generating undef for the top line in the buffering messages.  Also fixed display on fab4 to show them.

However I think we could consider cleaning the code below up now - the potentially bad elements in the array are likely to be userdata which evaluates to nil - representing undef in the perl array (feature of our luajson?).  In this case I think the correct approach is to treat it as an empty string, not removing it from the array as this moves text from one line to another...

--- On Tue, 16/2/10, Ben Klaas <bklaas@logitech.com> wrote:

> > From: Ben Klaas <bklaas@logitech.com>
> > Subject: Re: removing buffering showbrieflies for squeezeplay
> > To: "Triode" <triode1@btinternet.com>
> > Date: Tuesday, 16 February, 2010, 20:04
> > combining then splitting has the
> > distinct smell of a workaround for inconsistent server data,
> > and looking further that's what I'm seeing. Sometimes \n
> > comes in strings, sometimes it comes as an array.
> > 
> > local function _formatShowBrieflyText(msg)
> >    
> >    log:debug("_formatShowBrieflyText")
> > 
> >        -- showBrieflyText needs to
> > deal with both \n instructions within a string
> >        -- and also adding newlines
> > between table elements
> > 
> >        -- table msg needs to have
> > elements of only strings/numbers so table.concat will work
> >        for i, v in ipairs(msg) do
> >            
> >    if type(v) ~= 'string' and type(v) ~=
> > 'number' then
> >                
> >        table.remove(msg, i)
> >            
> >    end
> >        end
> > 
> >        -- first compress the table
> > elements into a single string with newlines
> >        local text =
> > table.concat(msg, "\n")
> >        -- then split the new
> > string on \n instructions within the concatenated string,
> > and into a table
> >        local split =
> > string.split('\\n', text)
> >        -- then compress the new
> > table into a string with all newlines as needed
> >        local text2 =
> > table.concat(split, "\n")
> > 
> >        return text2
> > end
> > 
> > Triode wrote:
>> > > I think the problem is I'm seeing is due to combining
> > and splitting the two lines of text the server sends within
> > squeezeplay.  If you combine and slit with \n as the
> > separator then a single line on the lower line will be
> > treated as the top line.  In the case of the two line
> > display this means that the title get lost.  Why not
> > send the original array from the show briefly message to
> > playerTitleStatus? 
>> > > --- On Tue, 16/2/10, Ben Klaas <bklaas@logitech.com>
> > wrote:
>> > > 
>> > >   
>>> > >> From: Ben Klaas <bklaas@logitech.com>
>>> > >> Subject: Re: removing buffering showbrieflies for
> > squeezeplay
>>> > >> To: "Triode" <triode1@btinternet.com>
>>> > >> Date: Tuesday, 16 February, 2010, 19:44
>>> > >> I'm not sure then...maybe there's a
>>> > >> bug to be worked out
>>> > >> 
>>> > >> from Player.lua:
>>> > >> 
>>> > >>           
> >    elseif type == 'song' then
>>> > >>             
> >        
> > self.jnt:notify('playerTitleStatus', self,
>>> > >> textValue, duration)
>>> > >>             
> >       The intended design is that if a
> > showBriefly
>>> > >> comes over with a type of 'song', send a
> > playerTitleStatus
>>> > >> notification out. The NowPlaying applet has a
> > method for
>>> > >> that notification, and should display them in the
> > title
>>> > >> bar.
>>> > >> 
>>> > >> Update-- I just checked and I do see these
> > messages on
>>> > >> Radio, but apparently not on Touch. I'll open a
> > bug.
>>> > >> 
>>> > >> Triode wrote:
>>> > >>     
>>>> > >>> Well I don't see them....  What is shown
> > and when
>>>> > >>>       
>>> > >> - is it only $status?  If so for my case
> > there's
>>> > >> nothing in that which possibly explains it
> > (iplayer spends a
>>> > >> long time "connecting")  We don't need to
> > send the rest
>>> > >> of the data if this is all that is shown.
>>> > >>     
>>>> > >>> This does not explain why the main title does
> > not
>>>> > >>>       
>>> > >> update until after the showbriefies have finished
> > though.
>>> > >>     
>>>> > >>> --- On Tue, 16/2/10, Ben Klaas <bklaas@logitech.com>
>>>> > >>>       
>>> > >> wrote:
>>> > >>     
>>>> > >>>          
>>>>> > >>>> From: Ben Klaas <bklaas@logitech.com>
>>>>> > >>>> Subject: Re: removing buffering
> > showbrieflies for
>>>>> > >>>>         
>>> > >> squeezeplay
>>> > >>     
>>>>> > >>>> To: "Triode" <triode1@btinternet.com>
>>>>> > >>>> Cc: andy@slimdevices.com
>>>>> > >>>> Date: Tuesday, 16 February, 2010, 19:08
>>>>> > >>>> That's not true actually, buffering
>>>>> > >>>> messages are shown in the title bar of the
> > Now
>>>>> > >>>>         
>>> > >> Playing
>>> > >>     
>>>>> > >>>> screen, just not with intrusive
> > showBrieflies...
>>>>> > >>>> 
>>>>> > >>>> Triode wrote:
>>>>> > >>>>           
> >   
>>>>>> > >>>>> Ben,
>>>>>> > >>>>> 
>>>>>> > >>>>> Given that a displaystatus message of
> > type
>>>>>> > >>>>>       
> >    
>>> > >> song
>>> > >>     
>>>>>> > >>>>>         
> >         
>>>>> > >>>> doesn't seem to result in anything being
> > shown on
>>>>> > >>>>         
>>> > >> the
>>> > >>     
>>>>> > >>>> screen, I'd like to propose we don't send
> > the
>>>>> > >>>>         
>>> > >> buffering
>>> > >>     
>>>>> > >>>> updates to the player.
>>>>> > >>>>           
> >   
>>>>>> > >>>>> This has two benefits:
>>>>>> > >>>>> 1) less wasted comet traffic
>>>>>> > >>>>> 2) it allows the new title to be
> > displayed
>>>>>> > >>>>>       
> >    
>>> > >> immediately
>>> > >>     
>>>>>> > >>>>>         
> >         
>>>>> > >>>> rather than waiting for the buffing
> > messages to
>>>>> > >>>>         
>>> > >> stop (which
>>> > >>     
>>>>> > >>>> I am seeing with iPlayer)
>>>>> > >>>>           
> >   
>>>>>> > >>>>> Views?
>>>>>> > >>>>> 
>>>>>> > >>>>> Index: Slim/Player/Player.pm
>>>>>> > >>>>> 
>>>>>> > >>>>>         
> >         
>>> > >>
> > ===================================================================
>>> > >>     
>>>>> > >>>>           
> >   
>>>>>> > >>>>> --- Slim/Player/Player..pm 
> >              
>>> > >>        
>>>>> > >>>>     (revision 30169)
>>>>> > >>>>           
> >   
>>>>>> > >>>>> +++ Slim/Player/Player.pm   
> >            
>>> > >>        
>>>>> > >>>>     (working copy)
>>>>> > >>>>           
> >   
>>>>>> > >>>>> @@ -1210,7 +1210,7 @@
>>>>>> > >>>>>         
> >             
>>> > >>           
> >    
>>>>> > >>>> $client->display->updateMode(0);
>>>>> > >>>>           
> >   
>>>>>> > >>>>>         
> >             
>>> > >>           
> >    
>>>>> > >>>> $client->showBriefly({
>>>>> > >>>>           
> >   
>>>>>> > >>>>>         
> >             
>>> > >>           
> >    
>>>>> > >>>>          line
> > => [
>>>>> > >>>>         
>>> > >> $line1, $line2 ],
>>> > >>     
>>>>> > >>>>           
> >   
>>>>>> > >>>>> -         
> >            
>>> > >>            
>>>>> > >>>>       
> >    jive => {
>>>>> > >>>>         
>>> > >> type
>>> > >>     
>>>>> > >>>> => 'song', text => [ $status,
>>>>> > >>>>         
>>> > >> $args->{'title'} ],
>>> > >>     
>>>>> > >>>> 'icon-id' => $args->{'cover'},
> > duration
>>>>> > >>>>         
>>> > >> => 500 },
>>> > >>     
>>>>> > >>>>           
> >   
>>>>>> > >>>>> +         
> >            
>>> > >>            
>>>>> > >>>>       
> >    jive =>
>>>>> > >>>>         
>>> > >> undef,
>>> > >>     
>>>>> > >>>>           
> >   
>>>>>> > >>>>>         
> >             
>>> > >>           
> >    
>>>>> > >>>>         
> > cli         
>>> > >> => undef,
>>> > >>     
>>>>> > >>>>           
> >   
>>>>>> > >>>>>         
> >             
>>> > >>           
> >    
>>>>> > >>>> }, { duration => 1, block => 1 });
>>>>> > >>>>           
> >   
>>>>>> > >>>>>         
> >         
>>>>> > >>>>           
> >   
>>>> > >>>          
>>> > >>     
>> > > 
>> > >   
> > 
> >