lyrics: Restore GTK support #151

radioactiveman · 2024-02-01T00:08:35Z

Make it possible to reuse most code with the Qt plugin
Rename to lyrics since lyricwiki is no longer used as source

radioactiveman · 2024-02-01T00:13:14Z

@jlindgren90: This is still a draft, but I would like to get some feedback and also help regarding Autotools.

It is possible to use a single Makefile and directory? My current approach with referencing relative paths is not ideal. Something like this does not work unfortunately.

SRCS = chart_lyrics_provider.cc \
       file_provider.cc \
       lyrics_ovh_provider.cc \
       utils.cc

include ../../buildsys.mk
include ../../extra.mk

plugindir := ${plugindir}/${GENERAL_PLUGIN_DIR}

LD = ${CXX}

CFLAGS += ${PLUGIN_CFLAGS}

ifeq ($(USE_GTK),yes)
PLUGIN = lyrics${PLUGIN_SUFFIX}
SRCS += lyrics.cc
CPPFLAGS += ${PLUGIN_CPPFLAGS} ${GTK_CFLAGS} ${GLIB_CFLAGS} ${JSON_GLIB_CFLAGS} ${XML_CFLAGS} -I../..
LIBS += ${GTK_LIBS} ${GLIB_LIBS} ${JSON_GLIB_LIBS} ${XML_LIBS} -laudgui
endif

ifeq ($(USE_QT),yes)
PLUGIN = lyrics-qt${PLUGIN_SUFFIX}
SRCS += lyrics-qt.cc
CPPFLAGS += ${PLUGIN_CPPFLAGS} ${QT_CFLAGS} ${GLIB_CFLAGS} ${XML_CFLAGS} -I../..
LIBS += ${QT_LIBS} ${GLIB_LIBS} ${XML_LIBS}
endif

radioactiveman · 2024-02-01T00:32:55Z

src/lyrics/lyrics-qt.cc

+    WidgetLabel (N_("<b>Local Storage</b>")),
+    WidgetCheck (N_("Load lyric files (.lrc) from local storage"),
+        WidgetBool (CFG_SECTION, "enable-file-provider"))
+};


Could defaults and widgets be also shared between Qt and GTK somehow?

Sure, you could put them in preferences.h. They can be static and don't need to be members of Lyrics or LyricsQt.

radioactiveman · 2024-02-01T00:34:51Z

src/lyrics/lyrics-qt.cc

+    QJsonDocument doc = QJsonDocument::fromJson (json);
+
+    if (doc.isNull () || ! doc.isObject ())
+        return false;


Should we call output = String(); in error cases explicitly?

I don't think that's necessary. It's fine to leave output unchanged in error cases.

radioactiveman · 2024-02-01T00:36:14Z

src/lyrics/lyrics.cc

+    WidgetLabel (N_("<b>Local Storage</b>")),
+    WidgetCheck (N_("Load lyric files (.lrc) from local storage"),
+        WidgetBool (CFG_SECTION, "enable-file-provider"))
+};


libaudgui does not show all widgets for some reason. This needs further verification.

It looks like libaudgui does not support nested WidgetTable. See fill_table() in prefs-widget.cc.

src/lyrics/lyrics.cc

radioactiveman · 2024-02-01T00:38:02Z

src/lyrics/lyrics.cc

+            if (edit_uri && edit_uri[0])
+                append_item_to_menu (menu, _("Edit Lyrics ..."),
+                 (GCallback) edit_lyrics_cb, aud::to_ptr ((const char *) edit_uri));
+        }


How would you pass edit_uri to the callback function?

I would make a copy of the string with g_strdup(), then instead of g_signal_connect() use g_signal_connect_data() and pass g_free() as the destroy_data parameter.

https://docs.gtk.org/gobject/func.signal_connect_data.html

radioactiveman · 2024-02-01T00:39:45Z

src/lyrics/lyrics.h

+        None,
+        Embedded,
+        Local,
+        LyricWiki, // TODO: Can this be safely removed?


It is no longer used, but affects the value of entries below. Can we remove it anyway?

I think we can remove LyricWiki. It doesn't look like enum Source is used directly in the config file.

jlindgren90 · 2024-02-02T02:58:15Z

Thanks. I'll take a look at this when I can and try to answer the specific questions. I know in advance it will be good quality, your work always is :)

radioactiveman · 2024-02-03T16:21:11Z

src/lyrics/lyrics.cc

+
+            // TODO: Are these reassignments safe and not leaking memory?
+            artist = CharPtr (truncate_by_pattern (artist, artist_pattern));
+            title = CharPtr (truncate_by_pattern (title, title_pattern));


I assume the results of g_match_info_fetch() are leaked here. Is this correct? If yes, what's the cleanest way to free and reassign the strings?

This is actually fine, nothing is leaked. CharPtr has overloaded operator= which handles freeing the previous pointer.

radioactiveman · 2024-02-03T16:28:42Z

Thanks. I'll take a look at this when I can and try to answer the specific questions. I know in advance it will be good quality, your work always is :)

Thanks John. :) It's sufficient if you review the GTK plugin plus the shared utils and answer my questions. All other code was extracted from the Qt plugin and is unchanged.

When I know how to structure the directory/files, I will split the changes into multiple commits (prepare code sharing + restore GTK plugin).

jlindgren90 · 2024-02-10T03:10:38Z

For directory structure, what you have already is okay, but my suggestion is to have 3 folders:

src/lyrics-common contains the toolkit-agnostic source files (but no Makefile etc.)
src/lyrics-gtk contains the GTK-specific source file (lyrics-gtk.cc) and the Makefile etc. for the GTK plugin
src/lyrics-qt contains the Qt-specific source file (lyrics-qt.cc) and the Makefile etc. for the Qt plugin

It's fine for the Makefiles to use relative paths to access the source files in src/lyrics-common. This is what we've done already with e.g. the src/ui-common folder.

jlindgren90 · 2024-02-10T03:18:04Z

I guess an advantage of your current approach is that there's just a single meson.build. I unfortunately don't know an easy way to do anything similar (i.e. use a single folder and Makefile) with the buildsys.mk system. It seems to assume one Makefile per plugin.

I think I'd slightly prefer to split the meson.build and use 3 folders as I described above. It's a little more hassle but I think it's less confusing in the end. Your call.

It is no longer available as mingw32 package. Workaround: Skip this package until we find a fix for our Windows autotools pipeline to succeed on mingw64. See also: https://www.msys2.org/news/#2023-12-13-starting-to-drop-some-32-bit-packages

Also remove all lyricwiki references since it is no longer used.

- Based on the original code of Audacious 4.0 - Backport all new features from the Qt lyrics plugin - Use new dependency "json-glib" to parse JSON strings

radioactiveman · 2024-02-12T02:35:13Z

Thanks again @jlindgren90 for the review and suggestions. I went ahead and merged it myself with the suggested changes.

If you have an idea how to make autotools work on mingw64 (see pull request #152), please let me know.
For now I have added a workaround: 98be4f1

radioactiveman requested a review from jlindgren90 February 1, 2024 00:08

radioactiveman commented Feb 1, 2024

View reviewed changes

radioactiveman force-pushed the lyrics-plugin branch from d919bd6 to 8cb90d4 Compare February 3, 2024 16:17

radioactiveman commented Feb 3, 2024

View reviewed changes

radioactiveman force-pushed the lyrics-plugin branch from 8cb90d4 to 344ba72 Compare February 3, 2024 19:59

radioactiveman force-pushed the lyrics-plugin branch from 344ba72 to e8243ee Compare February 12, 2024 00:19

radioactiveman added 4 commits February 12, 2024 03:02

lyricwiki-qt: Rename to lyrics-qt

7e0b9b0

Also remove all lyricwiki references since it is no longer used.

lyrics-qt: Split code into multiple reusable files

a0dff14

lyrics-gtk: Restore lyrics plugin for GTK

5793bd8

- Based on the original code of Audacious 4.0 - Backport all new features from the Qt lyrics plugin - Use new dependency "json-glib" to parse JSON strings

radioactiveman force-pushed the lyrics-plugin branch from e8243ee to 5793bd8 Compare February 12, 2024 02:10

radioactiveman marked this pull request as ready for review February 12, 2024 02:19

radioactiveman changed the title ~~WIP: lyrics: Restore GTK support~~ lyrics: Restore GTK support Feb 12, 2024

radioactiveman merged commit e33ce89 into audacious-media-player:master Feb 12, 2024
8 checks passed

radioactiveman deleted the lyrics-plugin branch February 12, 2024 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lyrics: Restore GTK support #151

lyrics: Restore GTK support #151

radioactiveman commented Feb 1, 2024

radioactiveman commented Feb 1, 2024

radioactiveman Feb 1, 2024

jlindgren90 Feb 10, 2024

radioactiveman Feb 1, 2024

jlindgren90 Feb 10, 2024

radioactiveman Feb 1, 2024

jlindgren90 Feb 10, 2024

radioactiveman Feb 1, 2024

jlindgren90 Feb 10, 2024

radioactiveman Feb 1, 2024

jlindgren90 Feb 10, 2024

jlindgren90 commented Feb 2, 2024

radioactiveman Feb 3, 2024

jlindgren90 Feb 10, 2024

radioactiveman commented Feb 3, 2024 •

edited

Loading

jlindgren90 commented Feb 10, 2024

jlindgren90 commented Feb 10, 2024

radioactiveman commented Feb 12, 2024

lyrics: Restore GTK support #151

lyrics: Restore GTK support #151

Conversation

radioactiveman commented Feb 1, 2024

radioactiveman commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlindgren90 commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radioactiveman commented Feb 3, 2024 • edited Loading

jlindgren90 commented Feb 10, 2024

jlindgren90 commented Feb 10, 2024

radioactiveman commented Feb 12, 2024

radioactiveman commented Feb 3, 2024 •

edited

Loading