Re: [emms-help] [Yoni Rabkin] Re: Patch: emms-playlist-tracks-in-region

[Yoni Rabkin]

Fran Burstall <address@hidden> writes:

> Have a look at branch emms-playlist-limit for a re-write of
> emms-playlist-limit.el.  This is my first piece of elisp for public
> consumption so criticism very welcome and almost certainly necessary.
>
> The main difference is that the limiting is done by a playlist-in,
> playlist-out function (see emms-playlist-limit--limit-playlist) that
> approximates Yoni's derived playlist idea.
>
> Along the way, I fixed various bugs caused by 
>
> * cursor not on a track
> * current track not set in limit playlist
>
> that caused the previous implementation to error out or otherwise
> misbehave.

This is all good. I'm fine with you merging this into master so that
everyone can play with it (especially considering that it already
compiles with no errors or warnings.)

Once you do we need to:

    * write documentation (I'm volunteering to do that)

    * document the change in one sentence in the NEWS file (you do that)

    * add you to the AUTHORS file (you do that too)

> As with the previous implementation, limiting always applies to the
> current playlist and makes the new limited playlist current.  I
> wonder if that is the optimal behaviour?
>
> ---Fran

That's a good question. The confusion here would be that you could be
trying to limit a playlist buffer and keep on getting an error that no
tracks are being found because the code is looking for tracks from
_this_ buffer in the _current_ buffer:

    1. create two playlist buffers "classical" and "prog rock"

    2. make "classical" the current playlist buffer

    3. switch to "prog rock" (note we are just switching to it, not
       making it current)

    4. try to limit to a track from "prog rock" (say, "21st Centry
       Schizoid Man")

    5. get an error because "21st Centry Schizoid Man" can't be found in
       the "classical" buffer

The behavior isn't technically an error, but I think it falls short of
the Do What I Mean principle.

What do you think?

> On Mon, 24 Sep 2018 at 18:21, Yoni Rabkin <address@hidden> wrote:
>
>    
>    
>    
>     ---------- Forwarded message ----------
>     From: Yoni Rabkin <address@hidden>
>     To:address@hidden
>     Cc: 
>     Bcc: 
>     Date: Thu, 20 Sep 2018 14:27:18 -0400
>     Subject: Re: [emms-help] Patch: emms-playlist-tracks-in-region
>     "Fran Burstall (Gmail)" <address@hidden> writes:
>    
>     > Hi,
>     >
>     > Some news: I made good progress on a more robust implementation
>     but
>     > hit a wall when I realised when I realised that I had not
>     understood
>     > how emms handles multiple playlist buffers.
>    
>     I'm happy to hear you've been working on it.
>    
>     > My idea had been that limiting by artist (say) creates a new
>     playlist
>     > buffer which was not necessarily current (so I can limit to Led
>     > Zeppelin while still listening to whatever is playing in the
>     current
>     > unlimited buffer). 
>    
>     I would say that this is the right direction. But maybe we've
>     been
>     thinking about this in a manner which is too... limited: perhaps
>     we can
>     first implement an `emms-playlist-derived' mechanism. This would
>     be a
>     general mechanism which takes as input playlist-A, applies a
>     series of
>     functions to it and outputs playlist-B as a result. Then we could
>     implement `emms-playlist-limit' using `emms-playlist-derived'.
>    
>     This would allow people to then use `emms-playlist-derived' to
>     implement
>     other interesting playlist modification functions (Emacs already
>     uses
>     this technique to define modes with `define-derived-mode'.) For
>     instance, code could use `emms-playlist-derived' to iterate over
>     a
>     playlist and split it into multiple playlists which are
>     per-artist or
>     album.
>    
>     > I implemented a mechanism for edits (deletions, maybe also
>     insertions)
>     > to the limited buffer to propagate back to the source buffer. 
>     > However, then I realised that some emms buffer operations,
>     notably
>     > emms-playlist-save, apply to the current playlist even when
>     that is
>     > not be the playlist buffer from which it is called.   So, for
>     example,
>     > I cannot save a new Led Zeppelin playlist to disc from my
>     limited
>     > buffer unless I first make it current.
>    
>     I would recommend against such a mechanism; operations on one
>     playlist
>     buffer shouldn't affect another.
>    
>     But perhaps I don't fully understand what type of operations you
>     wanted
>     to do with the limited buffer. Stuff like narrow-to-region?
>    
>     > At this point I lost momentum because I could not decide
>     whether the
>     > limited playlist should automatically become current. 
>    
>     In such a case my policy is to choose a default behavior (in this
>     case I
>     would say that the default would be that the new buffer would be
>     made
>     current) but build in the option so that people can change the
>     behavior. For example, a `emms-playlist-limit-new-current-p'
>     variable
>     set by default to t. But see more on this below.
>    
>     > I wanted to be able to limit arbitrary (non-current) playlists
>     as an
>     > aid for editing such playlists but this does not seem such a
>     good fit
>     > with how emms works (at least according to my current
>     understanding).
>    
>     This makes me think that when invoked pragmatically (which can be
>     implicit from the user's perspective) the function shouldn't
>     change the
>     current playlist buffer unless an argument is passed to the
>     function
>     saying otherwise, but when invoked interactively (which is an
>     explicit
>     action by the user) it would obey
>     `emms-playlist-limit-new-current-p' as
>     above.
>    
>     > So advice would be very welcome here!  How do you want the
>     limit
>     > feature to work? 
>    
>     I hope my thoughts above make some sense. Also, do you mind if I
>     post
>     this latest conversation onto the mailing list so that:
>    
>         * everyone else can read and weigh-in
>    
>         * we can continue the conversation there, where it will be
>     archived
>    
>     P.S. Please feel free to create a remote branch on the repo once
>     you
>     have code so that we can poke at it as well.
>    
>     > ---Fran
>     >
>     >
>     >
>     >
>     >
>     > On Wed, 19 Sep 2018 at 19:16, Yoni Rabkin <address@hidden>
>     wrote:
>     >
>     >   
>     >     "Fran Burstall (Gmail)" <address@hidden> writes:
>     >   
>     >     > I agree that the current implementation is a little
>     fragile, to
>     >     say
>     >     > the least!
>     >     >
>     >     > If the re-implementation is a long way down the to-do
>     list,
>     >     would it
>     >     > be helpful if I had a go at doing it?  (It won't hurt my
>     >     feelings if
>     >     > you say no!)
>     >     >
>     >     > ---Fran
>     >   
>     >     Any news on this front?
>     >   
>     >     >
>     >     > On Thu, 21 Jun 2018 at 02:39, Yoni Rabkin <
>     address@hidden>
>     >     wrote:
>     >     >
>     >     >     Fran Burstall <address@hidden> writes:
>     >     >   
>     >     >     > The (not very important) issue: in a playlist
>     buffer I
>     >     hit
>     >     >     > // (emms-playlist-limit-to-all).
>     >     >     >
>     >     >     > What I expect to happen: nothing if the playlist is
>     >     already
>     >     >     > unlimited.
>     >     >     >
>     >     >     > What happens: the playlist has its order reversed.
>     >     >     >
>     >     >     > Commentary: emms-playlist-limit-to-all refills the
>     >     playlist
>     >     >     > from a list created by a previous application of
>     >     >     > emms-playlist-tracks-in-region.  The latter lists
>     the
>     >     tracks of
>     >     >     the
>     >     >     > region in reverse order on account of just consing
>     up
>     >     said list
>     >     >     > from the top.
>     >     >     >
>     >     >     > Please consider applying the following tiny patch
>     to
>     >     >     > emms-playlist-tracks-in-region.
>     >     >     >
>     >     >     > ---Fran
>     >     >     >
>     >     >     > diff --git a/lisp/emms.el b/lisp/emms.el
>     >     >     > index 062b6eb..7a14a9e 100644
>     >     >     > --- a/lisp/emms.el
>     >     >     > +++ b/lisp/emms.el
>     >     >     > @@ -1098,7 +1098,7 @@ This is supplying ARGS as
>     arguments
>     >     to
>     >     >     the source."
>     >     >     >        (emms-walk-tracks
>     >     >     >          (setq tracks (cons (emms-playlist-track-at
>     >     (point))
>     >     >     >                             tracks))))
>     >     >     > -    tracks))
>     >     >     > +    (nreverse tracks)))
>     >     >     > 
>     >     >     >  (defun emms-playlist-track-updated (track)
>     >     >     >    "Update TRACK in all playlist buffers."
>     >     >   
>     >     >     I think that the way emms-playlist-limit currently
>     works is
>     >     >     essentially
>     >     >     a kludge (indeed, the very idea behind
>     >     `emms-playlist-limit' is a
>     >     >     strange way of replicating a subset of the browser.)
>     >     Storing the
>     >     >     entire
>     >     >     playlist in a variable is bad. We already have a data
>     >     structure
>     >     >     for
>     >     >     storing playlists: it's called a playlist!
>     >     >   
>     >     >     Once we re-write it, these issues will go away by
>     dint of
>     >     >     emms-playlist-limit being re-implemented correctly.
>     For
>     >     example,
>     >     >     we
>     >     >     could make it so that limiting creates a new playlist
>     with
>     >     the
>     >     >     selected
>     >     >     tracks and makes that playlist the current playlist.
>     If the
>     >     >     original
>     >     >     playlist was called "Classical" and was limited to
>     the
>     >     artist
>     >     >     Beethoven,
>     >     >     the new playlist would be generated as "Classical
>     (limited
>     >     to
>     >     >     artist/Beethoven)". The new playlist can have a local
>     >     variable
>     >     >     pointing
>     >     >     to the original playlist so that a "//" can be
>     equivalent
>     >     to
>     >     >     "kill this
>     >     >     buffer and make the original playlist current (if it
>     still
>     >     >     exists)".
>     >     >   
>     >     >     I'm sure that there are other implementation ideas
>     which
>     >     are as
>     >     >     good.
>     >     >   
>     >     >     --
>     >     >        "Cut your own wood and it will warm you twice"
>     >     >
>     >     >
>     >     >
>     >   
>     >     --
>     >        "Cut your own wood and it will warm you twice"
>     >
>     >
>     >
>    
>     --
>        "Cut your own wood and it will warm you twice"
>    
>    
>    
>     ---------- Forwarded message ----------
>     From: Yoni Rabkin <address@hidden>
>     To: emms-help mailing list <address@hidden>
>     Cc: 
>     Bcc: 
>     Date: Mon, 24 Sep 2018 13:20:42 -0400
>     Subject: [emms-help] [Yoni Rabkin] Re: Patch:
>     emms-playlist-tracks-in-region
>    
>    
>     --
>        "Cut your own wood and it will warm you twice"
>     _______________________________________________
>     Emms-help mailing list
>     address@hidden
>     https://lists.gnu.org/mailman/listinfo/emms-help
>
>
>

-- 
   "Cut your own wood and it will warm you twice"