Re: [emms-help] [PATCH] Improve browsing by artist and year

[Petteri Hintsanen]

On 19/10/15 16:41, Rasmus wrote:
> > +(defcustom emms-browser-prefer-albumartist nil
> > +  "*Prefer albumartist tag.
> > +If non-nil, emms-browser-get-track-field-extended prefers
> > +albumartist tag over artist tag, if available."
> > +  :group 'emms-browser
> > +  :type 'boolean)
>
> Please make this t.  I'd rather call the next EMMS v5 than hold back
> improvements.
...
> > +(defcustom emms-browser-prefer-originalyear nil
>
> It’s not obvious that this should be off by default.  Presumably, /if/ you
> have this tag, it should be used as long as the normal year tag is used as
> fallback .

OK, maybe both should be t by default.  I'm usually very conservative 
about the defaults.  Backwards compatibility is valuable.

> > +(defun emms-browser-get-track-field-extended (track type)
> IMO: This should be merged with emms-browser-get-track-field.  As long as
> the preferred type of year and artist can be set there’s no reason to have
> two functions.

emms-get-track-field is a wrapper that just funcalls 
emms-browser-get-track-field-function.  According to the docstring, 
emms-browser-get-track-field-function is meant for "customizing the way 
data is organized in the browser."  As such I think that it is 
appropriate to provide extended functionality via separate function, 
hence emms-browser-get-track-field-extended.  It needs to be documented 
though.

First I was thinking about modifying emms-track-get directly to replace 
year with originalyeal and artist with albumartist there (if prefer 
variables are non-nil).  But I think that it would be too invasive 
change.  (Compatibility worry again.)

> > +(defun emms-browser-extract-year-from-date (date)
> > +  "Try to extract year part from DATE.
> > +Return DATE if the year cannot be extracted."
> > +  (or (nth 5 (parse-time-string date))
> > +      (when (string-match "^[ \t]*[0-9]\\{4\\}" date)
>
> Is there any reason for limiting only look a fields starting with the
> year?  Can we trust that parse-time-string always catches arbitrary
> formats?  Alternatively you could look for just 4 consecutive numbers,
> though this may lead to errors.  As you prefer.

I believe your guess is as good as mine.  I don't have any data for this.

> > -  "Return a string representation of a track number.
> > -The string will end in a space. If no track number is available,
> > +  "Return a string representation of a disc number.
> > +The string will end in a space. If no disc number is available,
>
> This is a probably a separate bug fix commit.

Ouch, that got in there by mistake, sorry.

Thanks,
Petteri