/gnome-shell-extension-appindicator

statusNotifierWatcher: seek for SNIs in the bus to ensure we have them all #97

Merged
merged 5 commits into from Oct 11, 2017

Conversation

Reviewers

@jhasse jhasse

Assignees

@3v1n0 3v1n0

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@3v1n0 @jhasse @seb128 @romansimakov @lestcape
@3v1n0
Collaborator

3v1n0 commented Oct 10, 2017 •
Edited 1 time

Some indicators (coff, dropbox, coff) do not re-register again
when the plugin is enabled/disabled, thus we need to manually look
for the objects in the session bus that implements the
StatusNotifierItem interface...

This fixes issue #75, workarounding the applications problem.

Including also some other cleanups.

This won't apply to bugged apps that are confined (i.e. in snaps) since we won't be able to introspect them. The only option with these is trying to access to the standard /StatusNotifyItem path and see if properties are set. Less priority for now, though.

@3v1n0 3v1n0 self-assigned this Oct 10, 2017

@jhasse
Collaborator

jhasse commented Oct 10, 2017

Awesome stuff, I'll check this out later :)

Regarding the Copyright though: There are contributions not by Canonical. Maybe we should create an AUTHORS.txt instead?
statusNotifierWatcher.js
RegisterStatusNotifierItemAsync: function(params, invocation) {
// it would be too easy if all application behaved the same
// instead, ayatana patched gnome apps to send a path
// while kde apps send a bus name
- let service = params[0];
+ let [service] = params;
@jhasse

jhasse Oct 10, 2017

Collaborator

Sorry, I'm not a JavaScript expert. Are these two lines equivalent?

@3v1n0

3v1n0 Oct 10, 2017

Collaborator

Yep: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

@jhasse

jhasse Oct 10, 2017

Collaborator

What happens if params has two elements?

statusNotifierWatcher.js
}
- let id = this._getItemId(bus_name, obj_path);
+ print("AppIndicatorSupport-PRINT Registering",bus_name,obj_path)
@jhasse

jhasse Oct 10, 2017

Collaborator

Use Util.Logger.debug instead.

@3v1n0

3v1n0 Oct 10, 2017

Collaborator

This wasn't supposed to be pushed, but... Sure.

@jhasse
Collaborator

jhasse commented Oct 10, 2017

Commit messages:

statusNotifierWatcher: -> StatusNotifierWatcher:
remoove -> remove
You can leave out the Util:/Utils: prefix.
@3v1n0
Collaborator

3v1n0 commented Oct 10, 2017 •
Edited 1 time

statusNotifierWatcher: -> StatusNotifierWatcher:

I tend to use the same file basename, but as you whish..

remoove -> remove

Damned typos :)

You can leave out the Util:/Utils: prefix.

You say? I'd prefer to always have a context, but...
@jhasse
Collaborator

jhasse commented Oct 10, 2017

I tend to use the same file basename, but as you whish..

Ups my bad: I didn't see that the file was named statusNotifierWatcher.js. Nevermind then ;)

You say? I'd prefer to always have a context, but...

Makes sense in most cases, but if you look at the history of commits, you'll see that nearly no one has done it. "Util" wouldn't help me for example and for the files touched - Git already tells me that ;)

3v1n0 added some commits Aug 21, 2017

@3v1n0
statusNotifierWatcher: separate the items addition in different funct… 
…ions

Plus fix the names of the signals emitted to use the right ones.
Add Canonical Ltd. copyright
5bec865
@3v1n0
statusNotifierWatcher: use the unique name for registering indicators 
Otherwise an app could have different names registered, and we could end up
in two indicators for the same content.
f8c439f
@3v1n0
statusNotifierWatcher: ensure we try to use a valid parameter as obje… 
…ct name

why hoping when we can check?
e8b65df
@3v1n0
Add functions to traverse dbus and filter objects 
These allows to go through the exported objects and do something with them
if they match some filter we define.

This might not work properly when objects are confined, but in that case
we might only have a solution for items with a well-known path.
34516c4
@3v1n0
statusNotifierWatcher: seek for SNIs in the bus to ensure we have the… 
…m all

Some indicators (*coff*, dropbox, *coff*) do not re-register again
when the plugin is enabled/disabled, thus we need to manually look
for the objects in the session bus that implements the
StatusNotifierItem interface...
acf57f0
@3v1n0
Collaborator

3v1n0 commented Oct 10, 2017

Ok, fixed commit messages...
@jhasse
Collaborator

jhasse commented Oct 11, 2017 •
Edited 1 time

I've tested this with GNOME 3.24 and it works perfectly, good job!

Still I wouldn't want the Copyright (C) 2017 Canonical Ltd. at the top. First of all this is wrong, as non-Cononical-employees have edited the files, too, and also this might give a wrong impression that contribution requires signing some sort of CLA or something. Could you replace it with your name?
@seb128

seb128 commented Oct 11, 2017 •
Edited 1 time

@jhasse

The Copyright Canonical is only one of the lines, Marco didn't delete the other copyright mentions, why would it give the impression that Canonical owns the whole copyright for that source?

Also giving copyright to Canonical for work done by Canonical employees during their work time is the company policy and pretty standard, if you look at GNOME you will see that Red Hat does the same.

I'm not going to enter an argument about CLA but Canonical employees have always been contributing to upstream projects under the rules of the project they contribute to and I think if you look at the record there has been number of contributions made to GNOME and other upstream projects without any issue.
@seb128

seb128 commented Oct 11, 2017

if that can make feel better here is an example of Canonical copyright in upstream GNOME in glib
https://git.gnome.org/browse/glib/tree/gio/gsettingsschema.c#n3
@jhasse
Collaborator

jhasse commented Oct 11, 2017 •
Edited 1 time

It doesn't give the impression that Canonical owns the copyright for the whole source, but the copyright for everything that happened in 2017. I admit that's my fault for not touching the copyright lines after taking over from Jonas.

As those copyright lines don't include all copyright holders anyway (which are everyone who ever touched the file and/or the company they worked for) I would rather remove them entirely than adding another entry.
@seb128

seb128 commented Oct 11, 2017

"copyright forall the 2017 changes" is a bit of a weird notion, nothing is telling you when reading the source which lines got edited when, if you want that info you need to go through the commit logs which include the author for the commits...

if you prefer removing the copyright holders from the individual sources and maintain a contributors/authors file that's your choice as the maintainer, could that be done in a following commit though? The Ubuntu freeze for this cycle is tomorrow and we would like to get the changes merged so we can do a proper backport and land that in Ubuntu tomorrow.
@jhasse
Collaborator

jhasse commented Oct 11, 2017

if you prefer removing the copyright holders from the individual sources and maintain a contributors/authors file that's your choice as the maintainer, could that be done in a following commit though? The Ubuntu freeze for this cycle is tomorrow and we would like to get the changes merged so we can do a proper backport and land that in Ubuntu tomorrow.

Of course, I just wanted to discuss this before I would touch the copyright line ;)

I'll create an AUTHORS.md file right now, please create a new issue if you disagree with the way I'll do it.

@jhasse jhasse merged commit 63e468d into Ubuntu:master Oct 11, 2017

@seb128

seb128 commented Oct 11, 2017

great, thanks!

@thermatk thermatk referenced this pull request Oct 27, 2017

Closed

Some icons disappear when shell is restarted or the session is locked and unlocked #75

@romansimakov

romansimakov commented Oct 27, 2017

Sorry for maybe offtopic. Can I expert the fix in the next Ubuntu 17.10 updates or what I can to to get the fix?
@3v1n0
Collaborator

3v1n0 commented Oct 27, 2017

Yeah, an update is coming and under SRU review

@lestcape lestcape referenced this pull request in linuxmint/Cinnamon Oct 29, 2017

Open

Hidden tray icons #2846

@lestcape

lestcape commented on acf57f0 Dec 26, 2017 •
Edited 1 time

This is actually a pandemic... The current Qt implementation of com.canonical.AppMenu.Registrar also is not watching the own and unknown name of the dbus iface. So, is not called again after a gnome session restart. Interesting workaround.

Edit: Just with the difference that there are a lot of com.canonical.dbusmenu and they are not a menubar and also, there are not a way to recover the window xid from the Dbus objects with that interface, just the sender and the path can be recovered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment