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 30 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 prettier@2.8.8
 
 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 prettier@2.8.8
 
 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.

josselinonduty and others added 2 commits January 20, 2025 17:00
@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 added the enhancement New feature or request label Jan 31, 2025
@josselinonduty
Copy link
Collaborator Author

Anyone using arm64 by any chance?

@josselinonduty
Copy link
Collaborator Author

I removed support for snap because of AppArmor restrictions and refactored the docs. Added contributing guidelines and good practices.

@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
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
| ----------------------- | ----------------------------------------------------------------------------------------------- |
| `--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 <contact@josselinonduty.fr>
Date: Fri, 24 Jan 2025 16:03:43 +0100
Subject: [PATCH] fix: ensure os release is valid

based on commit by Dorian Stoll <dorian.stoll@tmsp.io>

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 <contact@josselinonduty.fr>
Date: Fri, 24 Jan 2025 16:03:43 +0100
Subject: [PATCH] fix: ensure os release is valid

based on commit by Dorian Stoll <dorian.stoll@tmsp.io>

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

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