Add originaldate and originalyear fields #49
Conversation
|
My patches to MPD and libmpdclient have been merged. I've rebased this on master. Ready for review and merge! |
|
Rebased again. |
There was a problem hiding this comment.
Unsure about the actual implementation. Does this field shadow the original date field? Are anyone interested in seeing the re-issue year instead of the original year of release? If yes, then differing between these two fields as you have done here is a good choice. If no, it should be an option (set originaldate).
| set nonextafteraction | ||
|
|
||
| set scrolloff=4 | ||
|
|
||
| set libraryroot=/home/bjn/media/music/ | ||
|
|
||
| set topbar=%ifcursong%%time_elapsed% %playstate% %time% (%progresspercentage%%%)%endif%\t%ifcursong%%artist%%endif%\tVol: %volume%%% Mode: %muteshort%%consumeshort%%repeatshort%%randomshort%%singleshort%\n%listsize%\t%ifcursong%%album% (%year%)%endif%\tQ: %livequeuesize%\n\t%ifcursong%==> %title% <==%endif% | ||
| set topbar=%ifcursong%%time_elapsed% %playstate% %time% (%progresspercentage%%%)%endif%\t%ifcursong%%artist%%endif%\tVol: %volume%%% Mode: %muteshort%%consumeshort%%repeatshort%%randomshort%%singleshort%\n%listsize%\t%ifcursong%%album% (%originalyear% -> %year%)%endif%\tQ: %livequeuesize%\n\t%ifcursong%==> %title% <==%endif% |
There was a problem hiding this comment.
Is it necessary to clutter the display like this? IMO it would be preferable to use only one of them. Maybe originalyear?
There was a problem hiding this comment.
This is on an example rc file, which I use verbatim as my own config. This doesn't change anything default.
There was a problem hiding this comment.
Ah, of course. Sorry about the misunderstanding.
| fieldtypes->add("year", _("Year"), FIELD_YEAR, 5, sort_compare_year); | ||
| fieldtypes->add("originalyear", _("Orig"), FIELD_ORIGINALYEAR, 5, sort_compare_originalyear); |
There was a problem hiding this comment.
Unsure if Orig will be understood correctly if it is a column header. Isn't it better just to call it "Year"?
There was a problem hiding this comment.
It was the best four-or-fewer-letter string I could think up for this column which would differentiate it from "Year" when both are shown. It should be pretty obvious, I think, that it is showing years.
There was a problem hiding this comment.
If both are shown, then the difference in year will reveal which is the original.
There was a problem hiding this comment.
But then we might have two columns both titled "Year". I prefer them to be different, but will concede to your decision.
There was a problem hiding this comment.
It's not a decision but a question. I'm trying to figure out what would be best for the majority of users. As suggested earlier, an originaldate setting would resolve this ambiguity.
I suggest leaving it as it is ("Orig") and then, if wanted, implementing this as an option in the future.
There was a problem hiding this comment.
I meant "to whatever your decision shall be". :)
OK, sounds good to me. I'll have to spend a bit of time with it once a new MPD is released (I don't want to figure out cross-compiling to my Raspberry Pi, rather wait for a binary) to get a feel for what seems best, when my real music collection is displayed rather than the little test collection I used for development of this and the MPD patches. I suspect I will eventually agree with you that an option would be best.
| @@ -160,6 +167,13 @@ void Song::init() | |||
| year = ""; | |||
| } | |||
|
|
|||
| /* original year from original date */ | |||
There was a problem hiding this comment.
In case originalyear doesn't exist, should it be copied from year so that it will still display correctly?
There was a problem hiding this comment.
I don't think so. I imagine most people who want to do something with this field would use a combination of both, rather than simply preferring one over the other.
There was a problem hiding this comment.
I've often wished that the original year would display instead of the re-release year, so that doesn't apply to me unfortunately ;)
To resolve this, it could be implemented as an option. I'm not sure how much effort should be put into this old codebase though, with the new Go version is under development. I'll accept it as it is.
There was a problem hiding this comment.
I see your point. But for your purposes would sorting by originaldate and yet displaying only date (reissue) do the trick? You could display the originalyear as well as the year in your topbar, as I'm doing in my config. (I'd rather only show the originalyear if originaldate is different from date, but that'd require conditionals which we don't have.)
There was a problem hiding this comment.
Implementing the option would do the trick. I frankly don't think it's worth it at this stage though, so I suggest we leave it at that, and merge as-is.
It will sometimes exist and sometimes not, if that's what you mean. For sorting purposes I have
I personally want to see both. I have quite a few albums in their original release and also as a re-issue, and I want it to be obvious which is which, yet have them sorted right next to each other (even if other albums were released between the original and re-issue). Does that answer your questions? |
This shouldn't be merged until Max merges MusicPlayerDaemon/libmpdclient#1 and confirms it'll be part of libmpdclient 2.12 (and also my corresponding patch for MPD).
Just putting this here so it can be reviewed, for now.