francislavoie / gnome-shell-volume-scroller

GNOME Shell Extension for adjusting volume with a mouse wheel
https://extensions.gnome.org/extension/5904/volume-scroller/
GNU General Public License v2.0
6 stars 3 forks source link

Still not working in GNOME 45 #3

Closed peter-kehl closed 8 months ago

peter-kehl commented 8 months ago

Greetings on our Canadian Memorial day Francis,

Thank you for this extension. It has worked well on Manjaro (stable Arch) Linux pre-GNOME 45.

In GNOME 45 it was generated some error. The error disappeared today as I've updated to version 5 downloaded from GNOME - thank you: image

Now the errors are gone (or at least, not reported). However, now scrolling above the top panel doesn't change the volume.

The same with v. 12 ZIP from your releases on GitHub: image

If there's anything I can help with, please send steps/commands to run.

francislavoie commented 8 months ago

Dangit. I made some changes to use Math.clamp() which they added as a nonstandard helper, maybe I broke the math. See https://github.com/francislavoie/gnome-shell-volume-scroller/commit/1d49c7ae7e5d8bcea46762fca83848ab17765dca?w=1

I don't use Gnome 45 (I'm on Pop OS, Gnome 42) so I can't actually test it myself.

If you'd like to try to debug and fix it, you can add console.log lines in the code. You can hit Alt+F2 and type lg at the prompt to open LookingGlass which lets you see those logs. See https://github.com/francislavoie/gnome-shell-volume-scroller?tab=readme-ov-file#manual-installation

peter-kehl commented 8 months ago

Thanks for debugging tips - I haven't done any GNOME development before.

The problems doesn't seem to be related to Math.clamp. Or, if it is, I don't see anything logged in lg. All my local changes are: a version bump to 13, and adding console.log:

git diff
diff --git a/volume_scroller@francislavoie.github.io/extension.js b/volume_scroller@francislavoie.github.io/extension.js
index c69ad9f..2181586 100644
--- a/volume_scroller@francislavoie.github.io/extension.js
+++ b/volume_scroller@francislavoie.github.io/extension.js
@@ -13,6 +13,8 @@ const VolumeScrollerIcons = [

 export default class VolumeScrollerExtension extends Extension {
   enable() {
+    console.log("volume-scroller: enable started");
+    console.log("Math.clamp: " + typeof Math.clamp);
     this.settings = this.getSettings();
     const setGranularity = () => {
         this.volume_granularity = this.settings.get_int("granularity") / 100.0;
@@ -43,9 +45,11 @@ export default class VolumeScrollerExtension extends Extension {
       "default-sink-changed",
       this._handle_sink_change
     );
+    console.log("volume-scroller: enable finished");
   }

   disable() {
+    console.log("volume-scroller: disable started");
     this.settings = null;
     this.enabled = false;
     this.sink = null;
@@ -57,9 +61,11 @@ export default class VolumeScrollerExtension extends Extension {
     this.controller.disconnect(this.sink_binding);
     this.sink_binding = null;
     this.controller = null;
+    console.log("volume-scroller: disable finished");
   }

   _handle_scroll(_actor, event) {
+    console.log("volume-scroller: _handle_scroll started");
     let volume = this.sink.volume;

     switch (event.get_scroll_direction()) {
@@ -81,15 +87,17 @@ export default class VolumeScrollerExtension extends Extension {
     this.sink.push_volume();

     this._show_volume(volume);
-
+    console.log("volume-scroller: _handle_scroll finishing");
     return Clutter.EVENT_STOP;
   }

   _handle_sink_change(controller, id) {
+    console.log("volume-scroller: _handle_sink_change");
     this.sink = controller.lookup_stream_id(id);
   }

   _show_volume(volume) {
+    console.log("volume-scroller: _show_volume started");
     const percentage = volume / this.volume_max;
     const iconIndex = volume === 0
       ? 0
@@ -100,9 +108,11 @@ export default class VolumeScrollerExtension extends Extension {
     const label = this.sink.get_port().human_port;

     Main.osdWindowManager.show(monitor, icon, label, percentage);
+    console.log("volume-scroller: _show_volume finished");
   }

   _get_step() {
+    console.log("volume-scroller: _get_step");
     return this.volume_max * this.volume_granularity;
   }
 }
diff --git a/volume_scroller@francislavoie.github.io/metadata.json b/volume_scroller@francislavoie.github.io/metadata.json
index 33833dd..6b42fe8 100644
--- a/volume_scroller@francislavoie.github.io/metadata.json
+++ b/volume_scroller@francislavoie.github.io/metadata.json
@@ -8,5 +8,5 @@
   "url": "https://github.com/francislavoie/gnome-shell-volume-scroller",
   "uuid": "volume_scroller@francislavoie.github.io",
   "settings-schema": "org.gnome.shell.extensions.volume-scroller",
-  "version": 12
+  "version": 13
 }

image

But, nothing in Looking Glass: image

It may well be a GNOME issue. I've noticed that since the update, some GNOME functionality is very delayed. Usually it takes about a week for Manjaro to receive kernel/big fixes, so we'll see.

Any debugging tips or links are welcome.

francislavoie commented 8 months ago

:thinking: I think you could run journalctl -f | grep '\[EXTENSION\_LOG\]' to see the logs maybe? It's been a while since I did this, this extension, which was a fork, was my first gnome extension experience as well.

Edit: my bad, I misread this https://askubuntu.com/questions/788937/where-ubuntu-gnome-looking-glass-extensions-logs-are-stored you'd need to put some well-known string in all the journalctl logs then grep for that. [EXTENSION_LOG] has no special meaning.

There's more docs about debugging here: https://gjs.guide/extensions/development/debugging.html#logging

peter-kehl commented 8 months ago

Math.clamp does exist on Manjaro. The problem: this is null in let volume = this.sink.volume; in _handle_scroll(..). (Note that the following has it on line 69, because I added some console.log).

image image

Workaround: Assign this to a local variable (I've used self). There may be a more elegant I've tried a workaround, but I haven't used JS and JS closures for long...

Also, adding "use strict"; to the top of the JS file. That helps avoiding pitfalls. I'll provide a PR.

francislavoie commented 8 months ago

Aaaahh that makes sense. Better fix is .bind(this) on the closures before passing them to connect().

    this.scroll_binding = this.panel.connect(
      "scroll-event",
      this._handle_scroll.bind(this)
    );
    this.sink_binding = this.controller.connect(
      "default-sink-changed",
      this._handle_sink_change.bind(this)
    );

My fault, I forgot about that detail of JS. I tried simplifying (actor, event) => this._handle_scroll(actor, event) to this._handle_scroll because making a new closure that takes and passes the same args seems redundant, but the key detail is this binding between the two.

So either reverting that part from my earlier commit (to repeat the args) or using .bind(this) should work. I think the latter is cleaner because it's shorter and avoids having the list of args in 3 places in the same file.

Try both and see if they work :+1:

francislavoie commented 8 months ago

Also, I don't think it's necessary to add "use strict" because modules already have strict by default. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode#strict_mode_for_modules In fact, the fact that this is null is evidence that strict mode was already on, I think https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode#no_this_substitution

francislavoie commented 8 months ago

Some good reading about the possible options re this: https://stackoverflow.com/a/74369964/846934

francislavoie commented 8 months ago

Tagged https://github.com/francislavoie/gnome-shell-volume-scroller/releases/tag/v13