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

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

josselinonduty
Copy link

@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]
  • ensure build is successful
  • PR release
  • (test 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
Author

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

@josselinonduty
Copy link
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
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
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

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

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

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
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
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
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
Copy link
Author

reverting PR to draft until we agree on a solution to the previous problem

@randshell
Copy link
Contributor

hey! Big new update incoming, I see. 🙂

I didn't have the chance to test it properly, but the code LGTM. I'd just ask you about the removal of patches/remove-kernel-version-from-user-agent.patch, doesn't it affect all the requests and not just the one for the updater? If so, I think it'd best to have a more generic UA rather than the exact version.

As a side note, I still have issue #70, but if I'm the only one affected by it then I'm not sure what's the problem.

@josselinonduty
Copy link
Author

hey! Big new update incoming, I see. 🙂

I didn't have the chance to test it properly, but the code LGTM. I'd just ask you about the removal of patches/remove-kernel-version-from-user-agent.patch, doesn't it affect all the requests and not just the one for the updater? If so, I think it'd best to have a more generic UA rather than the exact version.

As a side note, I still have issue #70, but if I'm the only one affected by it then I'm not sure what's the problem.

I am trying again for user-agent patch.

For your issue #70, it is probably because you have a free plan (https://www.deezer.com/offers), which does not offer high fidelity audio.

@josselinonduty
Copy link
Author

josselinonduty commented Jan 20, 2025

Would this do as a dummy os ?

        var __module_os = require("os");
        __module_os.release = () => "10.0.26100.2894";
        __module_os.version = () => ""
        __module_os.platform = () => "win32";
        __module_os.type = () => "Windows_NT";
        module.exports = __module_os;

@randshell
Copy link
Contributor

randshell commented Jan 20, 2025

For your issue #70, it is probably because you have a free plan (https://www.deezer.com/offers), which does not offer high fidelity audio.

I had the Hi-Fi plan back then when I opened the issue, only now I got downgraded. Also, it wouldn't explain the complete disappearance of the dark mode.

Would this do as a dummy os ?

I think leaving it to Linux would be better, perhaps they we'll have an official version if they keep seeing Linux UA rather than if we were to fake a Windows install.

Actually, it looks from the old UA patch that it's exactly the UA that plays an important role in enabling some features or not, including dark mode. Have you had any chance to test this on Fedora 41? Perhaps this patch silently broke and this is why I'm seeing #70. From the patch by Dorian Stoll:

e.g: 5.11.0-16-generic works, while 5.15.14-200.fc35.x86_64 doesn't.

In fact, I have 6.12.9-200.fc41.x86_64.

PS. Thanks @StollD for documenting this!

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

Successfully merging this pull request may close these issues.

Deezer 7.0 update
4 participants