Add Playback History Qt-only plugin #138
Conversation
vedgy
force-pushed
the
add-history-plugin
branch
3 times, most recently
from
d3d32ed to
eedc773
Compare
|
Looks like a nice plugin, except some of the limitations seem quite significant:
I wonder how many users actually want a plugin that remembers only album history, and forgets it on exit? |
|
My use case is listening to music while not otherwise using computer (the display is off). Then being able to tell what was playing. Storing and loading history is useless then. Implementing storing and loading in an ini file is relatively easy. So if someone needs it, they can jump in. But there is a complication of automatic removal of old entries to limit the growth of history. Tracking individual songs calls for automatic history cleanup as well, because the history will grow faster. Plus plugin preferences has to be implemented to choose between album and track history. I don't want to implement either feature, because I'm not going to use them and can't be sure what exact behavior would be desired by the users who need these features. |
While it's not exactly what many people want, the feature can be somewhat useful to random-song users too. The playlist Entry Number of a first played song in an album is stored. Selecting a history entry selects this song in the playlist. Assuming the playlist is unmodified, one can follow the played songs. Except when consecutive songs happen to be picked from a common album. |
|
Will this plugin become mergeable if I add a song history option? A group box with two radio buttons |
|
Sorry for the delayed response, but I just haven't had time to review this so far. |
|
No problem. I'll wait for a review. |
vedgy
force-pushed
the
add-history-plugin
branch
from
40a16c9 to
0702921
Compare
|
Resolved trivial conflicts and rebased on master. Will integrate relevant changes since my last testing, if any, from the copied-and-modified plugin playlist-manager-qt. Then perhaps add the discussed song history option. EDIT: the Playback History plugin still works well and I found no relevant changes to integrate. I'll work on the song history option now. |
Copy and modify the playlist-manager-qt plugin code. A similar feature was requested in #553. However, this initial version of the playback history plugin tracks and displays only album history, not individual song/track history. Playback start/stop time is not tracked or shown either.
The implementation is straightforward, because audqt::TreeView already supports removing selected rows.
The added feature allows to follow playback history in the playlist without activating history entries, i.e. without affecting playback.
vedgy
force-pushed
the
add-history-plugin
branch
from
0702921 to
81605bb
Compare
|
Made a few changes that do not require retesting to the first 3 commits. Implemented the Song mode and preferences to switch between the Song and the Album modes. Tested everything again. Please review. I hope permanent history storage is not a requirement for merging. Implementing permanent storage would require adding a history size limit option and implementing automatic history cleanup. Maybe also changing the type of By the way, perhaps it makes sense to declare |
|
@radioactiveman can you review this? Not sure when I will be able to get to it. |
Yes, I will try out the plugin and review its code when finding some time. |
"view-history" is no specified icon name. Since I found no good alternative just don't set any. See also: https://specifications.freedesktop.org/icon-naming-spec/latest/
radioactiveman
merged commit 92d8487
into
audacious-media-player:master
|
Thanks @vedgy for your nice work on the new plugin. Happy New Year to you and Slava Ukraini. 🇺🇦 |
| @@ -90,7 +90,7 @@ static const audqt::MenuItem main_items[] = { | |||
|
|
|||
| static const audqt::MenuItem playback_items[] = { | |||
| audqt::MenuCommand ({N_("Song Info ..."), "dialog-information", "I"}, audqt::infowin_show_current), | |||
| audqt::MenuCommand ({N_("Playback History ..."), "view-history", "H"}, action_playback_history), | |||
| audqt::MenuCommand ({N_("Playback History ..."), nullptr, "H"}, action_playback_history), | |||
There was a problem hiding this comment.
locate view-history finds the view-history icon in 3 icon themes on my system: breeze, breeze-dark and oxygen. What happens when the icon is missing, as I assume is the case on your system?
There was a problem hiding this comment.
Then this icon seems to be KDE specific. When it is missing no icon is shown.
There was a problem hiding this comment.
If there is no harm in case of the system icon theme that does not have the icon, then why not show the icon on systems where it is available? The standard icon name list is rarely updated. As far as I know, popular non-standard icon names are adopted by icon themes informally. That's probably how it always worked before the standard was created.
EDIT: from the link in your commit message:
Publication Date: August 9 2007, Version: Version 0.8.90
There was a problem hiding this comment.
Fair enough, please check my two last commits from my fork. Can I merge them into the main repo?
There was a problem hiding this comment.
Both commits look good, thank you. Please merge.
By the way, defining QT_NO_CAST_FROM_ASCII project-wide is considered a good Qt practice. KDE projects define it by default. One benefit is that disabling implicit conversions forces developers to consider which encoding the string is in and whether it is translatable. If the string is not translatable, QLatin1String or QStringLiteral() can be used for optimization.
Copy and modify the playlist-manager-qt plugin code.
A similar feature was requested in #553. However, this initial version of the playback history plugin tracks and displays only album history, not individual song/track history. Playback start/stop time is not tracked or shown either.