emms-help
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Fran Burstall
Subject: Re: [emms-help] [Yoni Rabkin] Re: Patch: emms-playlist-tracks-in-region
Date: Mon, 24 Sep 2018 20:23:15 +0100

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.

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


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]