Spotify extension #43

BlueManCZ · 2021-01-27T16:08:21Z

New plugin available here: #138

Old extension:

I created a Spotify extension for Albert launcher. It allows user to search and play tracks, add them to the queue and choose particular Spotify client.

This code needs review and testing. It works without any problems for me, but there may be unpredictable situational problems, that I'm currently not aware of.

For all people willing to use this extension, please read README.md first. I describe there how to configure Spotify Web API connection in this extension.

It is QueryHandler::ExecutionType::Realtime and currently contains spotify and play triggers.

YouTube video: https://youtu.be/fCpvYUWIsBs

Instructions for building and running this PR:

# Clone repositories
git clone --recursive https://github.com/albertlauncher/albert.git

# Switch to this PR branch
cd albert/plugins
git fetch origin pull/43/head:spotify
git checkout spotify

# Build
mkdir ../build
cd ../build
cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_BUILD_TYPE=Debug
make -j5

# Run
./bin/albert -p "$(pwd)/lib/"

Bierchermuesli · 2021-07-19T20:40:59Z

nice! but do this extension depends on QT 5.15?

tmp/albert-source/plugins/spotify/src/extension.cpp:104:65: error: ‘textChanged’ is not a member of ‘QSpinBox’
  104 |     connect(d->widget->ui.spinBox_number_of_results, &QSpinBox::textChanged, [this](const QString &s){

qmake -v
QMake version 3.1
Using Qt version 5.12.8 in /usr/lib/x86_64-linux-gnu

lsb_release -a
No LSB modules are available.
Distributor ID:	Linuxmint
Description:	Linux Mint 20.2
Release:	20.2
Codename:	ulyssa

BlueManCZ · 2021-07-20T07:20:14Z

Hello! To be honest I don't know right now. I'm on vacation and I will take a look when I come back. 😉

BlueManCZ · 2021-07-24T17:57:36Z

@Bierchermuesli According to the docs, textChanged was introduced in Qt 5.14. Do you think it's a problem?

Bierchermuesli · 2021-07-25T09:13:37Z

just a Problem of outdated Qt version in some distros :-)
≤ Debian 10 or ≤ Ubuntu 20.04 (LTS) still have Qt 5.11.3.

Debian 11 (soon stable) has Qt 5.14 and Ubuntu >20.10 has Qt5.14

BlueManCZ · 2022-06-06T13:48:45Z

Is there anything I can do to help you merge this @ManuelSchneid3r? Or are you working on some breaking changes in the core that will require adaptation?

I've used this extension for more than one year and didn't notice a single problem.

ManuelSchneid3r · 2022-10-08T11:49:36Z

Do you think it's a problem?

currently albert is backward compatible until ubuntu 18.04 i.e. 5.9. 20.04 uses 5.12.. I strive to be one or two years backward compatible. Therefore I decided to always support the latest two ubuntu LTS. Unfortunately this means that the newest lib you can use is 5.12.

idkCpp

I'm very sorry, but this is in bad shape.

idkCpp · 2022-10-11T15:48:38Z

spotify/src/spotifyWebAPI.cpp

+    }
+
+    SpotifyWebAPI::~SpotifyWebAPI() {
+        delete manager;


You pass this as parent which should then take care of this. (Except when you leak it in the query handler)

idkCpp · 2022-10-11T15:50:42Z

spotify/src/extension.cpp

+    if (query->string().trimmed().isEmpty())
+        return;
+
+    d->api->manager = new QNetworkAccessManager();


Why is this happening? Also there is no parent specified. I strongly suspect a memory leak here.

Please use unique_ptr to avoid memory leaks

idkCpp · 2022-10-11T15:53:05Z

spotify/src/extension.cpp

+    d->api->manager = new QNetworkAccessManager();
+
+    // If there is no internet connection, make one alerting item to let the user know.
+    if (!d->api->testInternetConnection()) {


@ManuelSchneid3r When do we like network access in the handler? Is this an acceptable case? Wasn't there something about async queries? I forgot.

Also on a separate note; why test the connection? Nothing is gained here. Better to handle connection errors when they occur.

Well it is accessing a web api, hence it's necessary. I agree please use (short) timeouts

idkCpp · 2022-10-11T15:53:50Z

spotify/src/extension.cpp

+        auto filename = QString("%1/%2.jpeg").arg(d->cacheDirectory, track.albumId);
+
+        // Download cover image of the album.
+        d->api->downloadImage(track.imageUrl, filename);


There sure is a lot happening in the query handler...

Unfortunately yes, but in this case we can't get better than the lag introduces by the requests

idkCpp · 2022-10-11T15:57:10Z

spotify/src/extension.cpp

+    d->spotifyExecutable = settings().value("spotify_executable").toString();
+    d->cacheDirectory = settings().value("cache_directory").toString();
+
+    if (d->numberOfResults == 0) {


QSettings::value supports a default argument

idkCpp · 2022-10-11T16:05:17Z

spotify/src/spotifyWebAPI.cpp

+        connect(this, SIGNAL(deviceReady(QString, QString)), this, SLOT(play(QString, QString)));
+
+        QtConcurrent::run([=]() {
+            manager = new QNetworkAccessManager();


I assume you just plaster this line everywhere where you had a threading problem when testing... This really needs to be fixed. (Last time I mention threads and managers, but there are still more occurrences)

idkCpp · 2022-10-11T16:07:37Z

spotify/src/spotifyWebAPI.cpp

+        return jsonObject;
+    }
+
+    void SpotifyWebAPI::waitForSignal_(const QObject *sender, const char *signal) {


Looks like you don't want to go async.

idkCpp · 2022-10-11T16:08:55Z

spotify/src/spotifyWebAPI.cpp

+    }
+
+    QString SpotifyWebAPI::getFirstDeviceId() {
+        QVector<Device> *devices_ = getDevices();


This is leaked.

idkCpp · 2022-10-11T16:09:35Z

spotify/src/extension.cpp

+    auto results = d->api->searchTracks(query->string(), d->numberOfResults);
+
+    // Get available Spotify devices.
+    auto *devices = d->api->getDevices();


This is not only leaked, but its ownership is all over the place.

idkCpp · 2022-10-11T16:10:56Z

spotify/src/spotifyWebAPI.h

+    QString SEARCH_URL = "https://api.spotify.com/v1/search?q=%1&type=%2&limit=%3";
+    QString PLAY_URL = "https://api.spotify.com/v1/me/player/play?device_id=%1";
+    QString ADD_ITEM_URL = "https://api.spotify.com/v1/me/player/queue?uri=%1";
+    QString DEVICES_URL = "https://api.spotify.com/v1/me/player/devices";


These all look very const despite none of them are.

ManuelSchneid3r · 2022-12-27T10:33:54Z

Please port it to 0.18. Looking forward to finally use this one :D

ManuelSchneid3r · 2023-01-24T11:57:39Z

Will a subset of features work withou a premium account?

ManuelSchneid3r · 2023-03-29T15:44:57Z

@BlueManCZ do you still want this plugin to be merged?

BlueManCZ · 2023-03-29T16:14:14Z

Excuse me for not replying sooner.

I'm very sorry, but this is in bad shape.

Thank you for the code review @idkCpp, I will try to refactor this when porting to the new Albert core. You were right, that ton of managers was indeed a threading problem.

Please port it to 0.18. Looking forward to finally use this one :D

As soon as I get Qt6 and build working on Gentoo, I will try to port it.

Will a subset of features work without a premium account?

I'm not sure. I can't imagine what that subset would be. Based on the sentence from API documentation:
"This tutorial assumes you have a Spotify account (free or premium)."
maybe some will work. But you can't select a specific song to play without Spotify premium.

@BlueManCZ do you still want this plugin to be merged?

I do, but I still don't have Qt6 on Gentoo to port this to the new Albert core. But I keep this in mind and watch Telegram for news.

CaseOf · 2023-03-29T18:05:05Z

@BlueManCZ my ebuild for Qt6 Albert is ready and working. I just have to commit it, and do a pull request on gentoo repository.
It wont be merged quickly as I know them, and they have to review ebuilds for qtscxml and qhotkey too in the same PR.
You can manually add it to a local overlay for testing.

CaseOf · 2023-03-29T22:06:15Z

Here is my pull request on gentoo repository gentoo/gentoo#30405

ManuelSchneid3r · 2023-03-30T09:17:43Z

@BlueManCZ ill close this one for now. Please reopen or make a new one if it is time for it.

BlueManCZ added 13 commits January 10, 2021 11:37

Add extension configuration

4cabce3

Add basic UI settings

093769a

Add prototype

4801540

Add README.md

e98f0e5

Update README.md

9782a03

Fix white cover bug

7c595c9

Implement explicit content filter and results number limit

ac05931

Local client execution, internet connection test, cleanup

786a668

Add parents to object instances

4a9fe3b

Experimental QNetworkAccessManager re-creation in each thread

32f5e18

Suppress non-relevant threads, cleanup

be65c9c

Small fixes

0205f17

Add settings option to specify cache directory

fd8b9e8

ManuelSchneid3r force-pushed the master branch from a5d13d8 to ee55048 Compare October 28, 2021 07:57

Fix build option variable name

87c4b59

ManuelSchneid3r requested review from a team, tyilo, cyprienruffino, idkCpp, Surendrajat and CaseOf October 10, 2022 06:50

idkCpp suggested changes Oct 11, 2022

View reviewed changes

ManuelSchneid3r force-pushed the master branch from b991b95 to 3b125e5 Compare January 18, 2023 10:06

ManuelSchneid3r force-pushed the master branch 2 times, most recently from 2951883 to 9832cf1 Compare February 11, 2023 08:06

ManuelSchneid3r closed this Mar 30, 2023

BlueManCZ mentioned this pull request Nov 11, 2024

Spotify plugin #138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spotify extension #43

Spotify extension #43

BlueManCZ commented Jan 27, 2021 •

edited

Loading

Bierchermuesli commented Jul 19, 2021 •

edited

Loading

BlueManCZ commented Jul 20, 2021

BlueManCZ commented Jul 24, 2021

Bierchermuesli commented Jul 25, 2021

BlueManCZ commented Jun 6, 2022

ManuelSchneid3r commented Oct 8, 2022 •

edited

Loading

idkCpp left a comment

idkCpp Oct 11, 2022

idkCpp Oct 11, 2022

ManuelSchneid3r Oct 11, 2022

idkCpp Oct 11, 2022

ManuelSchneid3r Oct 11, 2022

idkCpp Oct 11, 2022

ManuelSchneid3r Oct 11, 2022

idkCpp Oct 11, 2022

idkCpp Oct 11, 2022

idkCpp Oct 11, 2022

idkCpp Oct 11, 2022

idkCpp Oct 11, 2022

idkCpp Oct 11, 2022

ManuelSchneid3r commented Dec 27, 2022

ManuelSchneid3r commented Jan 24, 2023

ManuelSchneid3r commented Mar 29, 2023

BlueManCZ commented Mar 29, 2023

CaseOf commented Mar 29, 2023 •

edited

Loading

CaseOf commented Mar 29, 2023

ManuelSchneid3r commented Mar 30, 2023

Spotify extension #43

Spotify extension #43

Conversation

BlueManCZ commented Jan 27, 2021 • edited Loading

New plugin available here: #138

Instructions for building and running this PR:

Bierchermuesli commented Jul 19, 2021 • edited Loading

BlueManCZ commented Jul 20, 2021

BlueManCZ commented Jul 24, 2021

Bierchermuesli commented Jul 25, 2021

BlueManCZ commented Jun 6, 2022

ManuelSchneid3r commented Oct 8, 2022 • edited Loading

idkCpp left a 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

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

ManuelSchneid3r commented Dec 27, 2022

ManuelSchneid3r commented Jan 24, 2023

ManuelSchneid3r commented Mar 29, 2023

BlueManCZ commented Mar 29, 2023

CaseOf commented Mar 29, 2023 • edited Loading

CaseOf commented Mar 29, 2023

ManuelSchneid3r commented Mar 30, 2023

BlueManCZ commented Jan 27, 2021 •

edited

Loading

Bierchermuesli commented Jul 19, 2021 •

edited

Loading

ManuelSchneid3r commented Oct 8, 2022 •

edited

Loading

CaseOf commented Mar 29, 2023 •

edited

Loading