Improve: Find on web #15477

daschuer · 2025-10-11T15:32:21Z

I was annoyed form by the Discogs lookup burried to deep in the track menu. The fix is to ad a "Last used" entry that is two levels above. Now the menu looks like this:

I have also removed the literal title and artist form the menu. That feels like clutter when using it.
I had an intermediate version with a tooltip instead but that was also disturbing.

What's next:
All these URL land on a ranked search result page. This requires an unnecessary extra click if there is a full match. Instead of going there we may do an API call and check if we have a full match and than proceed to the result immediately.

Now that a overpopulated "Find on Web" menu is mitigated, I am also tempted to add Youtube Music and Spotfy to it, because I am using it on regular basis. Is this OK do we want a plug in interface?

Genius, Shazam and Spotalike are also candidatures.

…n later.

…in the "Find on Web" Menu

acolombier · 2025-10-13T17:56:28Z

src/widget/findonweblast.cpp

+
+namespace {
+const QString kLibraryGroup = QStringLiteral("[Library]");
+const QString kFindOnWebLastActionKey = QStringLiteral("FindOnWebLastAction");


Should we use snake_case here, as this appears to be a newly introduced key?

acolombier · 2025-10-13T17:58:29Z

src/widget/findonweblast.cpp

+void FindOnWebLast::openInBrowser(const QUrl& url) {
+    if (!mixxx::DesktopHelper::openUrl(url)) {
+        qWarning() << "DesktopHelper::openUrl() failed for " << url;
+        DEBUG_ASSERT(false);


In other place, we have the pattern DEBUG_ASSERT(!"An assert message");. It is pretty useful if you are running a debug build with assert without a debugger attached to it, as it will show a message directly, which you can search for (or even debug from the message itself).
This is nit tho, as the file+line is displayed as well.

acolombier · 2025-10-13T17:59:20Z

src/widget/findonweblast.cpp

+        const QString& actionId,
+        const QString& actionText,
+        const QUrl& serviceUrl) {
+    qDebug() << "init()" << m_lastActionKey << actionId;


Do we want to keep this debug message? If yes, let's prefix with the class name to help identifying the message, as init is quite generic

Suggested change

qDebug() << "init()" << m_lastActionKey << actionId;

qDebug() << "FindOnWebLast::init()" << m_lastActionKey << actionId;

This is a debug leftover. Removed.

acolombier · 2025-10-13T18:00:08Z

src/widget/findonweblast.cpp

+    }
+
+    QStringView service = QStringView(actionId).left(firstCommaPos);
+    setText(tr("Find on %1: %2").arg(service, actionText));


suggestion

Suggested change

setText(tr("Find on %1: %2").arg(service, actionText));

setText(tr("Find %1 on %2").arg(actionText, service));

acolombier · 2025-10-13T18:00:41Z

src/widget/findonweblast.cpp

+    QStringView service = QStringView(actionId).left(firstCommaPos);
+    setText(tr("Find on %1: %2").arg(service, actionText));
+    disconnect();
+    connect(this, &QAction::triggered, this, [this, actionId, actionText, serviceUrl] {


you shouldn't need to capture those AFAIS

Suggested change

connect(this, &QAction::triggered, this, [this, actionId, actionText, serviceUrl] {

connect(this, &QAction::triggered, this, [this, serviceUrl] {

acolombier · 2025-10-13T18:01:20Z

src/widget/findonweblast.h

+    QString m_lastActionKey;
+};
+
+#pragma once


Copy paste left over?

Suggested change

#pragma once

acolombier · 2025-10-13T18:02:44Z

src/widget/findonwebmenufactory.h


-void createFindOnWebSubmenus(QMenu* pFindOnWebMenu, const Track& track);
+void createFindOnWebSubmenus(QMenu* pFindOnWebMenu,
+        FindOnWebLast* pFindOnWebLast,


I'm wondering if this should be a shared_ptr case - I assume you kept a fat pointer because Qt parented owned?

Yes, I have changed it to QPointer<>. At one point we have decided to use that, to pass a parented pointer around.

acolombier · 2025-10-13T18:05:57Z

src/widget/wcoverartmenu.cpp

 void WCoverArtMenu::createActions() {
-    m_pChange = new QAction(tr("Choose new cover",
-            "change cover art location"), this);
+    m_pChange = new QAction(tr("Choose file", "change cover art location"), this);


What's the rational for changing this? Feels like this will invalidate many translation but I'm not sure if the form is any better. Did we have some confusion with the previous wording?

We the old translation my expectation was to have a chooser dialog and not a file browser. The new text should avoid this. Since we are her in the main branch, optimizing texts is allowed.

…ection in the "Find on Web" Menu

daschuer · 2025-10-14T06:24:55Z

Done

daschuer · 2025-11-01T13:22:34Z

Code style fixed.

daschuer added 9 commits October 9, 2025 08:12

Resort Track->Metadata menu

3c27bc2

Improve text for Cover Art -> Choose file menu

765194d

Reorder Find on Web menu for fine to coarse, Exact Track to Cover Track

4622de3

Remove not working Artist + Album from LastFM

2fa8746

Remove title and track info from Find on Web menu to tidy it up.

f2770ae

Use a normal QMenu parent for the Find On Web root menu

1c85257

Move aboutToShow() code to updateMenus() to allow a first level actio…

89e1933

…n later.

Introduce a find on Web ID

70ef56e

Add another "Find on <Service>" menu the contains the last selection …

44797c2

…in the "Find on Web" Menu

github-actions bot added ui build labels Oct 11, 2025

daschuer mentioned this pull request Oct 12, 2025

Populate Find on web #15479

Open

acolombier reviewed Oct 13, 2025

View reviewed changes

fixup! Add another "Find on <Service>" menu the contains the last sel…

6e6b991

…ection in the "Find on Web" Menu

daschuer force-pushed the findonweb branch from 99a8134 to 3c8fb54 Compare October 14, 2025 20:44

Pass parented QObjects as QPointer

82a2e27

daschuer force-pushed the findonweb branch from 3c8fb54 to 82a2e27 Compare November 1, 2025 13:22

	qDebug() << "init()" << m_lastActionKey << actionId;
	qDebug() << "FindOnWebLast::init()" << m_lastActionKey << actionId;

	setText(tr("Find on %1: %2").arg(service, actionText));
	setText(tr("Find %1 on %2").arg(actionText, service));

	connect(this, &QAction::triggered, this, [this, actionId, actionText, serviceUrl] {
	connect(this, &QAction::triggered, this, [this, serviceUrl] {

Uh oh!

Improve: Find on web #15477

Are you sure you want to change the base?

Improve: Find on web #15477

Conversation

daschuer commented Oct 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Oct 14, 2025

Uh oh!

daschuer commented Nov 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants