Skip to content
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.

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

Release: Deezer v7 #95

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

josselinonduty
Copy link
Collaborator

@josselinonduty josselinonduty commented Jan 19, 2025

Bumps deezer version to 7.x.x.

  • update url/metadata
  • update deps
  • ensure patches are working as expected
    Removed: isDev (was included in updated codebase), AutoUpdater[1]
    • tray
    • mime type
    • mpris
    • user agent
  • ensure build is successful
  • PR release
  • testing on different archs

Feel free to propose other things to check before release

Closes #94

[1]: AutoUpdater might be fixable. We need to find a "platformVersion" variable that is used by semver/autoupdater, and then fake an api call (https://www.deezer.com/desktop/update?userId=1234&currentVersion=7.0.1&architecture=x64&platform=win32&platformVersion=<???>). However, since we only update the linux port through github/flatpak/anything but deezer, I think we do not care about this. Please let me know if you disagree.

@josselinonduty
Copy link
Collaborator Author

Some patches fail to be applied when node_modules are not already installed. Skipping patches for now..

@josselinonduty
Copy link
Collaborator Author

josselinonduty commented Jan 19, 2025

Driver error: libva error: i965_drv_video.so init failed
Fix: export LIBVA_DRIVER_NAME="iHD" using iHD driver (instead of i965)

Edit: after updating electron, I did not see any issue related to video drivers.

Output of vainfo:

$ vainfo
libva info: VA-API version 1.20.0
libva info: User environment variable requested driver 'iHD'
libva info: Trying to open /usr/lib/x86_64-linux-gnu/dri/iHD_drv_video.so
libva info: Found init function __vaDriverInit_1_20
libva info: va_openDriver() returns 0
vainfo: VA-API version: 1.20 (libva 2.12.0)
vainfo: Driver version: Intel iHD driver for Intel(R) Gen Graphics - 24.1.0 ()

@josselinonduty
Copy link
Collaborator Author

MVP: it works! 🥳

Now, I will bring back the necessary plugins - some of them are useless I believe (smn double check please)

@josselinonduty josselinonduty marked this pull request as ready for review January 19, 2025 23:55
@josselinonduty
Copy link
Collaborator Author

josselinonduty commented Jan 20, 2025

(@aunetx) @randshell @asyd could you review?

Also, this deprecates any work on v6.x.x
User PR: closes #91, closes #65
Bot PR: closes all previous GHA PRs

@asyd
Copy link
Collaborator

asyd commented Jan 20, 2025

@josselinonduty thanks for the PR! Mind to test if MPRIS is working fine? (you can test with dbus-monitor "type=signal,path=/org/mpris/MediaPlayer2" and check there is artUrl)

@josselinonduty
Copy link
Collaborator Author

I checked it was working based on the mpris desktop 'notification' (on ubuntu 24 / gnome 46 there is a persistent notification displaying the current track as well as the controls).

Anyway, here is the output of dbus-monitor "type=signal,path=/org/mpris/MediaPlayer2" after I skipped a track:

signal time=<timestamp> sender=:1.197 -> destination=(null destination) serial=10658 path=/org/mpris/MediaPlayer2; interface=org.freedesktop.DBus.Properties; member=PropertiesChanged
   string "org.mpris.MediaPlayer2.Player"
   array [
      dict entry(
         string "Metadata"
         variant             array [
               dict entry(
                  string "mpris:artUrl"
                  variant                      string "file:///tmp/.org.chromium.Chromium.<uid>"
               )
               dict entry(
                  string "mpris:length"
                  variant                      int64 199000000
               )
               dict entry(
                  string "mpris:trackid"
                  variant                      object path "/org/chromium/MediaPlayer2/TrackList/<trackid>"
               )
               dict entry(
                  string "xesam:album"
                  variant                      string "<album>"
               )
               dict entry(
                  string "xesam:artist"
                  variant                      array [
                        string "<artist>"
                     ]
               )
               dict entry(
                  string "xesam:title"
                  variant                      string "<title>"
               )
            ]
      )
   ]

@asyd
Copy link
Collaborator

asyd commented Jan 20, 2025

the artUrl is not good, I'll try to have a look once merged!

Thanks for your work!

@josselinonduty
Copy link
Collaborator Author

the artUrl is not good, I'll try to have a look once merged!

What do you expect the url to look like?

@Meincrakker
Copy link

Meincrakker commented Jan 20, 2025

Greetings I was able to build and run this on fedora. The discord rpc, mpris all worked for me.

The ArtUrl is just an url linking to a image. Your file shows a path on your computer. It is correct on my end though.

I made a patch that is probably not worth mentioning (I could not apply the patches without adding --fuzz and fixed some semantics):

install_build_deps:
-	@npm install --engine-strict asar
+	@npm install --engine-strict @electron/asar
 	@npm install [email protected]
 
 prepare: clean install_build_deps
@@ -27,7 +27,7 @@ prepare: clean install_build_deps
 	@cd source && 7z x -y -bsp0 -bso0 app-32.7z
 
 	@echo "Extract app sources from the app"
-	@node_modules/asar/bin/asar.js extract source/resources/app.asar app
+	@node_modules/@electron/asar/bin/asar.js extract source/resources/app.asar app
 
 	@echo "Prettier the sources to patch successfully"
 	@node_modules/prettier/bin-prettier.js --write "app/build/*.js"
@@ -38,9 +38,9 @@ prepare: clean install_build_deps
 	@echo "Remove kernel version from User-Agent (https://github.com/aunetx/deezer-linux/pull/9)"
 	@echo "Avoid to set the text/html mime type (https://github.com/aunetx/deezer-linux/issues/13)"
 	@echo "Add a better management of MPRIS (https://github.com/aunetx/deezer-linux/pull/61)"
-	$(foreach p, $(wildcard ./patches/*), patch -p1 -dapp < $(p);)
+	$(foreach p, $(wildcard ./patches/*), patch -p1 -dapp --fuzz 3 < $(p);)
 
-	@echo "Append `package-append.json` to the `package.json` of the app"
+	@echo "Append package-append.json to the package.json of the app"
 	@echo "Adds electron, elecron-builder dependencies, prod and build directives"
 	@jq -s '.[0] * .[1]' app/package.json package-append.json > app/tmp.json && mv app/tmp.json app/package.json
 

Edit: the markdown formatting is killing me 😠

@asyd
Copy link
Collaborator

asyd commented Jan 20, 2025

the artUrl is not good, I'll try to have a look once merged!

What do you expect the url to look like?

Something like:

               dict entry(
                  string "mpris:artUrl"
                  variant                      string "https://cdn-images.dzcdn.net/images/cover/926394918acb3fb7d54d6c5786c7346f/250x250.jpg"
               )

@josselinonduty
Copy link
Collaborator Author

josselinonduty commented Jan 20, 2025

@Meincrakker

install_build_deps:
-	@npm install --engine-strict asar
+	@npm install --engine-strict @electron/asar
 	@npm install [email protected]
 
 prepare: clean install_build_deps
@@ -27,7 +27,7 @@ prepare: clean install_build_deps
 	@cd source && 7z x -y -bsp0 -bso0 app-32.7z
 
 	@echo "Extract app sources from the app"
-	@node_modules/asar/bin/asar.js extract source/resources/app.asar app
+	@node_modules/@electron/asar/bin/asar.js extract source/resources/app.asar app

Good idea! However, why would you create a patch for that?

+	$(foreach p, $(wildcard ./patches/*), patch -p1 -dapp --fuzz 3 < $(p);)

I believe that fuzzing can be somehow unreliable, so I personally advocate a real script update especially for a major release.

@Meincrakker
Copy link

@josselinonduty stupid question, how do you patch it on your pc?

I think it creates the package.json and lockfile dynamically when running make install_deps.

Also I messed up, because of markdown the ` disappeared. They should all be removed since it results into an error.

-	@echo "Append `package-append.json` to the `package.json` of the app"
+	@echo "Append package-append.json to the package.json of the app"

@josselinonduty
Copy link
Collaborator Author

josselinonduty commented Jan 20, 2025

@Meincrakker

@josselinonduty stupid question, how do you patch it on your pc?

I think it creates the package.json and lockfile dynamically when running make install_deps.

Also I messed up, because of markdown the ` disappeared. They should all be removed since it results into an error.

-	@echo "Append `package-append.json` to the `package.json` of the app"
+	@echo "Append package-append.json to the package.json of the app"

Edit: Actually, I read your message too quickly. I thought you wanted to change Deezer's deps after install_deps.
You were right.

Note: I am working on creating some docs and scripts to make patching more clear and easy.

@Meincrakker
Copy link

Meincrakker commented Jan 20, 2025

No worries :D

Nice! I am currently struggling with the electron-builder failing to compile an rpm. All other commands do work except this one ...

One thing I was very annoyed with is the notification tray, it has two "looks" and if you have flatpak installed you get the one with the black background, that also cannot display the menu at the correct location (that is a new bug) and needs an extension to be styled correctly. These bugs exist because of the extension that is required to get a systray in Gnome and outdated electron that requires a "legacy support" to work at all.
image
image

If you use a version that does not run in a sandbox and have "kf6-kstatusnotifieritem" installed it gets the new gtk design. And if it is removed electron uses the above design as a fallback. But then in the new design the play, next and previous buttons do not work.
image

I decided to sacrifice the button functionality for the outdated design and black box that can also stick to your background even if the app is closed.
image

Of course this will behave completely different on another Desktop Environment but for Gnome the simplest option would be to just remove the middle buttons and ship the flatpak with the dependency. Since with mpris working out of the box you really do not need to control the app via the system tray.

@josselinonduty
Copy link
Collaborator Author

josselinonduty commented Jan 20, 2025

Alright,

I would argue that mpris-service cannot be shipped anymore using electron@33.
I tested different versions of electron, but the app crashed instantly with electron<32 (electron 32 SEEMS to work).

In fact, I checked the changelog for electron v32 -> v33. They updated V8 engine to v13.0.0 which must have broken the api used by mpris-service (it uses the deprecated abstract-socket package causing the error).

There are 3 solutions for me:

  1. ship using electron@32: we may discover unwanted behaviours by downgrading the preferred electron version
  2. find/create an alternative to mpris-service that is up-to-date
  3. remove completely this patch, and use the default mpris metadata which is already sufficient for most uses.

I don't think solution 1 would be a good idea. I'd go for 3 (for now), but I figured out @asyd really loves mpris ;) (I agree it is nice to control deezer from cli and such) so you may want to fork and update abstract-worker/mpris-service if it is reasonable.

@josselinonduty josselinonduty marked this pull request as draft January 20, 2025 17:14
@josselinonduty josselinonduty marked this pull request as ready for review February 1, 2025 00:30
@josselinonduty josselinonduty requested review from aunetx, randshell and asyd and removed request for aunetx February 1, 2025 00:30
| ----------------------- | ----------------------------------------------------------------------------------------------- |
| `--start-in-tray` | Start the app in the tray (see [patch](./patches/01-start-hidden-in-tray.patch)) |
| `--disable-systray` | Quit the app when the window is closed (see [patch](./patches/03-quit.patch)) |
| `--disable-features` | Disable some features (see [patch](./patches/06-better-management-of-MPRIS.patch)) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no implementation of this arg.

Compared to Discord RPC, this one is ok as opt-out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is used in patches/06-better-management-of-MPRIS.patch, at the end of the patch. I am not sure I understood this switch well enough though, could you explain it ?

@randshell
Copy link
Collaborator

Hi!
Thanks for this @josselinonduty.

I gave this MR an initial read and left some comments. Other than what I wrote there, I'd like to add if we can add a short line inside the new patches to reference the original author? This is already Git, which keeps track of this aspect, but in my opinion it'd be a more appropriate way to maintain their credits.

Something along the lines of:

From 08ea86075592efe9c02c4beb7cbdfc6935ca0b1c Mon Sep 17 00:00:00 2001
From: josselinonduty <[email protected]>
Date: Fri, 24 Jan 2025 16:03:43 +0100
Subject: [PATCH] fix: ensure os release is valid

based on commit by Dorian Stoll <[email protected]>

What do you all think?

@josselinonduty
Copy link
Collaborator Author

Hi! Thanks for this @josselinonduty.

I gave this MR an initial read and left some comments. Other than what I wrote there, I'd like to add if we can add a short line inside the new patches to reference the original author? This is already Git, which keeps track of this aspect, but in my opinion it'd be a more appropriate way to maintain their credits.

Something along the lines of:

From 08ea86075592efe9c02c4beb7cbdfc6935ca0b1c Mon Sep 17 00:00:00 2001
From: josselinonduty <[email protected]>
Date: Fri, 24 Jan 2025 16:03:43 +0100
Subject: [PATCH] fix: ensure os release is valid

based on commit by Dorian Stoll <[email protected]>

What do you all think?

I could also redact the author from the file content, to keep all non-source related stuff in git metadata (like any git repo would do). What do you think?

 discord rpc; update rich-presence-builder; make discord rpc opt-in
@josselinonduty
Copy link
Collaborator Author

b745827: discord rich presence is now opt-in by default. Also, I dug around and found more metadata. I've added duration and track url to mpris and discord rich presence.

Note: I did not find any information in relation to position. seek, position for mpris, and exact track position for discord rpc cannot be implemented (unless I skipped sth).

@aunetx
Copy link
Owner

aunetx commented Feb 4, 2025

Hello @josselinonduty, @asyd and @randshell! your work seems amazing, thank you very much :)

I have a kind of weird question: I have sponsorship enabled for this project, and I sometimes receive a tiny bit of money for it (essentially $5/month I guess). However this is not really my work any more, so I have two solutions:

  • removing my sponsor informations from the project (that's all good for me)
  • giving you back what I get (it's not much but at least it's fair!)

It's not really about the money tbh but rather fairness for your work, so what do you prefer?

@josselinonduty
Copy link
Collaborator Author

Hello @josselinonduty, @asyd and @randshell! your work seems amazing, thank you very much :)

I have a kind of weird question: I have sponsorship enabled for this project, and I sometimes receive a tiny bit of money for it (essentially $5/month I guess). However this is not really my work any more, so I have two solutions:

  • removing my sponsor informations from the project (that's all good for me)
  • giving you back what I get (it's not much but at least it's fair!)

It's not really about the money tbh but rather fairness for your work, so what do you prefer?

I really don't care abt that. You can keep your links, you can remove them, I'm fine with any decision really

@loulou64490
Copy link

Can you give me your DNF build? I can't build it...

@josselinonduty
Copy link
Collaborator Author

Can you give me your DNF build? I can't build it...

Can reproduce.

System information:

  • clean install: Fedora-Workstation-Live-x86_64-41-1.4 in Virtual Box
  • Virtual Box v7.0.16_Ubuntu
  • make v4.4.1
  • 7z v16.02 (installed p7zip and p7zip-plugins)
  • gcc v14.2.1
  • g++ v14.2.1
  • libxcrypt-compat v4.4.38
  • rpm-build v4.20.0
  • patch v2.7.6
  • node.js v22.11.0
  • npm v10.9.0
    bold means missing from docs/clean install

I was able to build appimage, deb and tar on fedora. I was also able to build rpm on ubuntu. There must be a missing library for rpm build to build successfully on fedora.

Using this workaround referencing this I managed to build and run rpm on fedora.

@loulou64490
Copy link

loulou64490 commented Feb 9, 2025

Can you give me your DNF build? I can't build it...

Can reproduce.

System information:
...

I try the workaround and it works!!! I'm happy

@randshell
Copy link
Collaborator

I should have some time over the weekend to review it again. @josselinonduty could you maybe squash the commits that are no longer necessary? I think the various changes would be easier to understand this way.

On another note, I'm not sure if it's worth to leave the precise kernel version as the User Agent, but having code to truncate it to only the major version seems a bit overkill. Since the app is based on Electron and so there isn't really need for handling anything kernel related, what about we just keep a fixed version, like before, but one that isn't 0.0.0. For example, we could use 6.1 which is a LTS Kernel until like 2027 and also doesn't conflict with any Windows or recent MacOS version.

@randshell
Copy link
Collaborator

PS. I've closed the old PRs of the Flathub bot, since they are now superseded by this MR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deezer 7.0 update
6 participants