Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
51 stars 10 forks source link

Check for container to prevent occasional extension loading failure #45

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

as mentioned in this comment, I have been getting errors with the extension being unable to loads when testing previous version or order to investigate #42.

Not sure why we never had the issue before but I believe that this check should be included.

also updated the if () to include spaces as per gnome convention.

Batwam commented 1 year ago

any issue with this PR?

Moon-0xff commented 1 year ago

The error itself is kind of strange: this.container.get_parent() is null

How the parent container is null? To me it looks like a deeper problem with initialization or disabling of the extension. Proving that theory needs testing and I haven't done it.

It's also a very infrequent bug that it seems to only affect us as developers, so I don't think there's a need of fixing this quickly.

Batwam commented 1 year ago

Ok, no worries. The this.container.get_parent() is null error came up as I was trying older versions back-to-back to troubleshoot the Spotify issue. The code change fixed it and it seemed logical to check whether something existed before removing it. I thought it might help avoid other running into similar issues when upgrading the extension but it's hard to replicate.

I haven't seen it recently so I'm ok either way.

Batwam commented 1 year ago

Shall we close this suggestion?

Moon-0xff commented 1 year ago

I don't see why. The branch still merges cleanly and there's no indication that this is a non-issue now.

However, I don't have plans to test this soon. If it bothers you, you can close this PR and submit it as an issue. No problem.

Moon-0xff commented 1 year ago

I haven't been able to trigger this but now I can imagine how it could be triggered: If _updateTrayPosition is executed before the widget is added to the panel.

which is possible because we connect the signals before adding this to the panel:

                this.settings.connect('changed::left-padding',this._onPaddingChanged.bind(this));
                this.settings.connect('changed::right-padding',this._onPaddingChanged.bind(this));
                this.settings.connect('changed::extension-index',this._updateTrayPosition.bind(this));
                this.settings.connect('changed::extension-place',this._updateTrayPosition.bind(this));
                this.settings.connect('changed::show-icon',this._setIcon.bind(this));
                this.settings.connect('changed::use-album',this._setIcon.bind(this));
                this.settings.connect('changed::symbolic-source-icon', this._setIcon.bind(this));

                Main.panel.addToStatusArea('Mpris Label',this,EXTENSION_INDEX,EXTENSION_PLACE);

I will merge this then.