added lyrics navigation feature #465
Conversation
| void on_playback_dynamic_info_track(const file_info& info) override; | ||
| void on_playback_time(double /*time*/) override {} | ||
| void on_volume_change(float /*new_volume*/) override {} | ||
| const LyricData& get_lyrics() const { return m_lyrics; } // m_lyrics getter |
There was a problem hiding this comment.
This comment doesn't add any value.
| const LyricData& get_lyrics() const { return m_lyrics; } // m_lyrics getter | |
| const LyricData& get_lyrics() const { return m_lyrics; } |
| static UINT_PTR PANEL_UPDATE_TIMER = 2304692; | ||
|
|
||
| static std::vector<LyricPanel*> g_active_panels; | ||
| //static std::vector<LyricPanel*> g_active_panels; |
There was a problem hiding this comment.
To what end is this moved? Did you intend to remove this before submitting?
| #include "metadb_index_search_avoidance.h" | ||
| #include <vector> | ||
| class LyricPanel; | ||
| std::vector<LyricPanel*>& get_active_panels(); |
There was a problem hiding this comment.
I'm not super keen on returning the actual list of actual panels (and certainly not when neither of those things are const).
I wonder if there's a nice way to do this a little more directly...it feels like this should should be able to live in ui_hooks or somewhere like that...or that maybe there should be a central store for "the current lyrics"... I suppose until now most other use-cases have been powered by a particular panel (each of which already has their own lyrics on-hand).
I recognise this is possibly a difficult thing for a first-time contributor to reason about though because it'd be a very much a zoomed-out-high-level-code-organisation sort of refactoring.
I can see if I get some time to take a look at trying that though, and I'm open to ideas.
| case cmd_edit_lyrics: out = "Edit lyrics"; break; | ||
| case cmd_mark_instrumental: out = "Mark as instrumental"; break; | ||
| case cmd_seek_to_next_lyric: out = "Seek to next lyric timestamp"; break; | ||
| case cmd_seek_to_prev_lyric: out = "Seek to prev lyric timestamp"; break; |
There was a problem hiding this comment.
We shouldn't be using code-like abbreviations on the UI (same applies to the description down below).
| case cmd_seek_to_prev_lyric: out = "Seek to prev lyric timestamp"; break; | |
| case cmd_seek_to_prev_lyric: out = "Seek to previous lyric timestamp"; break; |
| case cmd_mark_instrumental: out = "Mark as instrumental"; break; | ||
| case cmd_seek_to_next_lyric: out = "Seek to next lyric timestamp"; break; | ||
| case cmd_seek_to_prev_lyric: out = "Seek to prev lyric timestamp"; break; | ||
| case cmd_seek_to_repeat_current_lyric: out = "Seek to repeat current lyric timestamp"; break; |
There was a problem hiding this comment.
I think a better name would be more to the effect of "Seek to the current lyric timestamp" because that's what it's doing. The repeat is a 2nd-order effect that only applies if it's currently playing.
| "Marking tracks as instrumental..."); | ||
| } break; | ||
| case cmd_seek_to_next_lyric: { | ||
| auto& panels = get_active_panels(); |
There was a problem hiding this comment.
| auto& panels = get_active_panels(); | |
| const auto& panels = get_active_panels(); |
| if (!panels.empty()) | ||
| { | ||
| LyricPanel* panel = panels[0]; | ||
| LyricData lyrics = panel->get_lyrics(); |
There was a problem hiding this comment.
We should try to avoid copying the entire LyricData here.
| LyricData lyrics = panel->get_lyrics(); | |
| const LyricData& lyrics = panel->get_lyrics(); |
|
|
||
| double timeNow = playback->playback_get_position(); | ||
|
|
||
| for (size_t i = lyrics.lines.size(); i-- > 0; ) |
There was a problem hiding this comment.
Looks like this is just std::lower_bound, could we use that to simplify all of these lookups?
| case cmd_edit_lyrics: | ||
| case cmd_seek_to_next_lyric: | ||
| case cmd_seek_to_prev_lyric: | ||
| case cmd_seek_to_repeat_current_lyric: |
There was a problem hiding this comment.
Hmm, to add to the reasons that it would be nice to not have to do the "get all panels, take one, check its lyrics" dance, we should disable these if there aren't any timestamped lyrics, and arguably we should disable the next/previous buttons if we're already on the last/first line respectively (or if not, make sure those buttons do sensible things in those cases, we don't want users to click a button that does nothing with no feedback).
2 participants
Implemented feature - lyrics navigation. Added 3 context menu buttons:
repeat current lyric timestampContext menu buttons can be assigned keyboard hotkeys
And then use it like this
VID_20250513_110916.1.mp4
Good for studying foreign languages.