Skip to content

added lyrics navigation feature #465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mokat6 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

added lyrics navigation feature #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 99 additions & 2 deletions src/ui_contextmenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "tag_util.h"
#include "ui_hooks.h"
#include "ui_util.h"
#include "ui_lyrics_panel.h"

static const GUID GUID_OPENLYRICS_CTX_POPUP = { 0x99cb0828, 0x6b73, 0x404f, { 0x95, 0xcd, 0x29, 0xca, 0x63, 0x50, 0x4c, 0xea } };
static const GUID GUID_OPENLYRICS_CTX_SUBGROUP = { 0x119bf93d, 0xdeec, 0x4fd2, { 0x80, 0xbb, 0x91, 0x6a, 0x58, 0x6a, 0x2, 0x25 } };
Expand All @@ -31,6 +32,9 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
case cmd_manualsearch_lyrics: out = "Search for lyrics (manually)"; break;
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;
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be using code-like abbreviations on the UI (same applies to the description down below).

Suggested change
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_seek_to_repeat_current_lyric: out = "Seek to repeat current lyric timestamp"; break;
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

default: uBugCheck();
}
}
Expand All @@ -47,6 +51,9 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
case cmd_show_lyrics:
case cmd_manualsearch_lyrics:
case cmd_edit_lyrics:
case cmd_seek_to_next_lyric:
case cmd_seek_to_prev_lyric:
case cmd_seek_to_repeat_current_lyric:
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
out_display_flags = contextmenu_item_simple::FLAG_DISABLED_GRAYED;
} break;
Expand Down Expand Up @@ -90,7 +97,7 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
io::search_for_lyrics(handle, true);
bool success = handle.wait_for_complete(30'000);
if(success)
{
{
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace is a bit wonky here and below.

if(handle.has_result())
{
const LyricData lyrics = handle.get_result();
Expand Down Expand Up @@ -142,7 +149,7 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
{
if(data.get_count() == 0) break;
metadb_handle_ptr track = data.get_item(0);

const auto async_edit = [track](threaded_process_status& /*status*/, abort_callback& abort)
{
const metadb_v2_rec_t track_info = get_full_metadata(track);
Expand Down Expand Up @@ -293,7 +300,79 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
core_api::get_main_window(),
"Marking tracks as instrumental...");
} break;
case cmd_seek_to_next_lyric: {
auto& panels = get_active_panels();
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto& panels = get_active_panels();
const auto& panels = get_active_panels();

if (!panels.empty())
{
LyricPanel* panel = panels[0];
LyricData lyrics = panel->get_lyrics();
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to avoid copying the entire LyricData here.

Suggested change
LyricData lyrics = panel->get_lyrics();
const LyricData& lyrics = panel->get_lyrics();

service_ptr_t<playback_control> playback = playback_control::get();

double timeNow = playback->playback_get_position();

for (size_t i = 0; i < lyrics.lines.size(); ++i)
{
double ts = lyrics.lines[i].timestamp;
if (ts > timeNow)
{
playback->playback_seek(ts);
break;
}
}
}
break;
}
case cmd_seek_to_prev_lyric: {
auto& panels = get_active_panels();
if (!panels.empty())
{
LyricPanel* panel = panels[0];
LyricData lyrics = panel->get_lyrics();
service_ptr_t<playback_control> playback = playback_control::get();

bool skipOne = true;
double timeNow = playback->playback_get_position();

for (size_t i = lyrics.lines.size(); i-- > 0; )
{
double ts = lyrics.lines[i].timestamp;
if (ts < timeNow)
{
if (skipOne)
{
skipOne = false;
continue;
}
playback->playback_seek(ts);
break;
}
}
}
break;
}
case cmd_seek_to_repeat_current_lyric: {
auto& panels = get_active_panels();
if (!panels.empty())
{
LyricPanel* panel = panels[0];
LyricData lyrics = panel->get_lyrics();
service_ptr_t<playback_control> playback = playback_control::get();

double timeNow = playback->playback_get_position();

for (size_t i = lyrics.lines.size(); i-- > 0; )
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is just std::lower_bound, could we use that to simplify all of these lookups?

{
double ts = lyrics.lines[i].timestamp;
if (ts < timeNow)
{
playback->playback_seek(ts);
break;
}
}
}
break;
}

default:
LOG_ERROR("Unexpected openlyrics context menu command: %d", int(index));
uBugCheck();
Expand All @@ -307,6 +386,9 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
static const GUID GUID_ITEM_MANUALSEARCH_LYRICS = { 0x9fac3e8e, 0xa847, 0x4b73, { 0x90, 0xe4, 0xc9, 0x5, 0x49, 0xf9, 0xe9, 0x32 } };
static const GUID GUID_ITEM_EDIT_LYRICS = { 0x518d992d, 0xd61b, 0x4cfd, { 0x8f, 0xcf, 0x8c, 0x7f, 0x21, 0xd0, 0x59, 0x2c } };
static const GUID GUID_ITEM_MARK_INSTRUMENTAL = { 0x23b658fc, 0x71e1, 0x4e3c, { 0x87, 0xe0, 0xb, 0x34, 0x8c, 0x26, 0x3f, 0x59 } };
static const GUID GUID_ITEM_SEEK_TO_NEXT_LYRIC = { 0x3e8f4dc9, 0xa3c9, 0x4f95, { 0x94, 0x47, 0x32, 0xc5, 0x6b, 0x1a, 0xe1, 0x6e } };
static const GUID GUID_ITEM_SEEK_TO_PREV_LYRIC = { 0x8b5a1a77, 0x4cc4, 0x4e1a, { 0x90, 0x39, 0xaa, 0xde, 0x3f, 0x48, 0x62, 0x1f } };
static const GUID GUID_ITEM_SEEK_TO_REPEAT_LYRIC = { 0xa7d1f6c2, 0x2bd5, 0x4a7b, { 0x8f, 0x6e, 0x3a, 0x91, 0x2b, 0xcc, 0xd4, 0x55 } };

switch(index)
{
Expand All @@ -315,6 +397,9 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
case cmd_manualsearch_lyrics: return GUID_ITEM_MANUALSEARCH_LYRICS;
case cmd_edit_lyrics: return GUID_ITEM_EDIT_LYRICS;
case cmd_mark_instrumental: return GUID_ITEM_MARK_INSTRUMENTAL;
case cmd_seek_to_next_lyric: return GUID_ITEM_SEEK_TO_NEXT_LYRIC;
case cmd_seek_to_prev_lyric: return GUID_ITEM_SEEK_TO_PREV_LYRIC;
case cmd_seek_to_repeat_current_lyric: return GUID_ITEM_SEEK_TO_REPEAT_LYRIC;
default: uBugCheck();
}
}
Expand All @@ -338,6 +423,15 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
case cmd_mark_instrumental:
out = "Remove existing lyrics and skip future automated lyric searches";
return true;
case cmd_seek_to_next_lyric:
out = "Seek to next lyrics timestamp";
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should see if we can add more information here or give a slightly more detailed description of what the command/button does (and similarly for the other options). For example:

Suggested change
out = "Seek to next lyrics timestamp";
out = "Seek to the timestamp of the next line of timestamped lyrics";

return true;
case cmd_seek_to_prev_lyric:
out = "Seek to prev lyrics timestamp";
return true;
case cmd_seek_to_repeat_current_lyric:
out = "Seek to current lyrics timestamp, repeat current line";
return true;
default:
uBugCheck();
}
Expand All @@ -351,6 +445,9 @@ class OpenLyricsContextSubItem : public contextmenu_item_simple
cmd_manualsearch_lyrics,
cmd_edit_lyrics,
cmd_mark_instrumental,
cmd_seek_to_next_lyric,
cmd_seek_to_prev_lyric,
cmd_seek_to_repeat_current_lyric,
cmd_total
};
};
Expand Down
8 changes: 7 additions & 1 deletion src/ui_lyrics_panel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,20 @@
#include "ui_lyrics_panel.h"
#include "ui_util.h"
#include "win32_util.h"
static std::vector<LyricPanel*> g_active_panels;

namespace {
// NOTE: This needs to be stored per-instance so that they all have their own
// timers, and stopping/starting doesn't fail when they conflict
// (e.g by having several panels all try to stop the same timer).
static UINT_PTR PANEL_UPDATE_TIMER = 2304692;

static std::vector<LyricPanel*> g_active_panels;
//static std::vector<LyricPanel*> g_active_panels;
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To what end is this moved? Did you intend to remove this before submitting?

}

std::vector<LyricPanel*>& get_active_panels()
{
return g_active_panels;
}

LyricPanel::LyricPanel() :
Expand Down
4 changes: 4 additions & 0 deletions src/ui_lyrics_panel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
#include "img_processing.h"
#include "lyric_io.h"
#include "metadb_index_search_avoidance.h"
#include <vector>
class LyricPanel;
std::vector<LyricPanel*>& get_active_panels();
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class LyricPanel : public CWindowImpl<LyricPanel>, protected ui_config_callback_impl, private play_callback
{
Expand All @@ -24,6 +27,7 @@ class LyricPanel : public CWindowImpl<LyricPanel>, protected ui_config_callback_
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
Copy link
Owner

@jacquesh jacquesh May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't add any value.

Suggested change
const LyricData& get_lyrics() const { return m_lyrics; } // m_lyrics getter
const LyricData& get_lyrics() const { return m_lyrics; }


CRect compute_background_image_rect();
void load_custom_background_image();
Expand Down
Loading