Review of attachment 289786 [details] [review]:

The patch is wrong - in _onStateChanged() you call setStyle(), but your new method is called updateRunningState(). I would also like to know what exactly you are trying to do - I'm not a big fan of this, in particular as I don't understand what the change facilitates. If you override setStyle()/updateRunningState() you don't need to disconnect the existing handler and connect a different one, but then if you want whatever change you make to take place immediately (rather than when an apps' state changes), you'd still have to mess around with existing app icons ...

Created attachment 289801 [details] [review]
Proposed patch

Fixed a typo.

The idea behind this change was that it would allow people to inject alternate functionality when there is a 'state' signal on the AppIcon without needing to connect/disconnect more signals.

Maybe there are better ways of going about this, but the idea came from when I was implementing a feature for the extension here: https://github.com/N-Yuki/gnome-shell-extension-workspace-isolated-dash
I wanted to intercept the _onStateChanged to only run this.actor.add_style_class_name('running'); if some other condition was met other than just (this.app.state != Shell.AppState.STOPPED), but this was not possible without injecting the modification before object construction due to how the signal was set up.

To get around this, I destroyed everything in Main.overview._dash._box and called _redisplay() on the AllView and FrequentView instances so all the AppIcon instances would be recreated, connecting the modified _onStateChanged.

(In reply to comment #3)
> I wanted to intercept the _onStateChanged to only run
> this.actor.add_style_class_name('running'); if some other condition was met
> other than just (this.app.state != Shell.AppState.STOPPED), but this was not
> possible without injecting the modification before object construction due to
> how the signal was set up.

But how does overriding updateRunningState() fix this for existing app icons? Your method would still only be executed on the first state change after the extension has been disabled.

(In reply to comment #4)
> (In reply to comment #3)
> > I wanted to intercept the _onStateChanged to only run
> > this.actor.add_style_class_name('running'); if some other condition was met
> > other than just (this.app.state != Shell.AppState.STOPPED), but this was not
> > possible without injecting the modification before object construction due to
> > how the signal was set up.
> 
> But how does overriding updateRunningState() fix this for existing app icons?
> Your method would still only be executed on the first state change after the
> extension has been disabled.

Okay, I admit that I haven't properly tested this change (I found another typo, so I'll upload a new revision soon) and I may be misunderstanding something since I haven't been able to get jhbuild working yet, but from my understanding of Javascript, how signals work, and testing this idea with examples inside LookingGlass tell me that when you have:
x.updateRunningStyle(...); // where x is an instance of AppIcon
it will run AppIcon.prototype.updateRunningStyle with 'this' being 'x'.

It also means that when I overwrite AppIcon.prototype.updateRunningStyle with a new function, all existing instances of AppIcon will now be running the updated function.

However, signals are a bit weird here since when it is connected, it resolves the function immediately, so any updates to AppIcon.prototype do not make a difference.
That's why we need this extra function call for an injection to take effect immediately on all existing instances.
I guess another way around this which doesn't involve creating another function could be changing:
Lang.bind(this, this._onStateChanged));
to:
Lang.bind(this, function() { this._onStateChanged(); });
in the connect call if that works.
In that case, I assume _onStateChanged would be renamed to onStateChanged to follow the current format(?)

Sorry if I said anything poor or if I'm just completely misunderstanding something.

Created attachment 289806 [details] [review]
Proposed patch

Forgot 'this.'

(In reply to comment #5)
> However, signals are a bit weird here since when it is connected, it resolves
> the function immediately, so any updates to AppIcon.prototype do not make a
> difference.

Yes. But what I'm saying:

> That's why we need this extra function call for an injection to take effect
> immediately on all existing instances.

... except that it doesn't! If appIcon0 is "running" when your extension is enabled, it will remain so independent from your injection until its app is stopped (or otherwise changes state).
So you'd still have to iterate over all existing icons to make sure your injection takes effect immediately, which means that you could just as well disconnect the existing function and reconnect with your injected handler.

(In reply to comment #7)
> (In reply to comment #5)
> > However, signals are a bit weird here since when it is connected, it resolves
> > the function immediately, so any updates to AppIcon.prototype do not make a
> > difference.
> 
> Yes. But what I'm saying:
> 
> > That's why we need this extra function call for an injection to take effect
> > immediately on all existing instances.
> 
> ... except that it doesn't! If appIcon0 is "running" when your extension is
> enabled, it will remain so independent from your injection until its app is
> stopped (or otherwise changes state).
> So you'd still have to iterate over all existing icons to make sure your
> injection takes effect immediately, which means that you could just as well
> disconnect the existing function and reconnect with your injected handler.

Right. Yes, I forgot about that.
You'd need something like:
Shell.AppSystem.get_default().get_running().forEach(function(app) {
	app.notify('state');
});
to ensure that the state signal is sent to make all the icons update to use the new style.
We don't need to signal the actual icons here, which would have been more troublesome.

I feel doing the above is still better than having to locate and recreate all the icons though, and it uses stable functions we can rely on (rather than having to dig through Main, and even inside Main, the location of icons/functions can vary between Shell versions since we're looking at instance variables).

If there's already a stable, easily accessible function that returns a list of all existing icons, then I would agree that the proposed patch is probably not as useful.

I guess a generalisation of this bug report would be that it is not simple to locate and update existing icons (and thus I resorted to just destroying and redisplaying it all after fiddling with LookingGlass and reading the source code to locate the stuff I needed for the versions of Shell I wanted to support).

So, I guess in summary, with the proposed patch, one would overwrite the new function and call app.notify('state') of all running applications.
If we instead had a function to return a list of existing icons, one would overwrite/inject _onStateChanged, reconnect for each icon, and call _onStateChanged for each icon. This is a little more work, and it relies on the signal connect inside AppIcon's constructor to not change variable names.

Maybe there's a better way that I haven't considered?

Review of attachment 289806 [details] [review]:

(In reply to comment #8)
> I guess a generalisation of this bug report would be that it is not simple to
> locate and update existing icons

s/icons/any class that sets up signal handlers in its constructor/

That's my main gripe here - it certainly doesn't hurt in this particular case, but I don't really fancy this as a general rule; it would just add unnecessary redirections all over the place just in case an extension might want to change something. Extensions have a lot of power to do crazy stuff, but it doesn't have to be necessarily easy :-)


> If we instead had a function to return a list of existing icons

Not sure what you mean here?

  Main.overview._dash._box.get_children().map(function(c) { return c.child._delegate; })

(if you also want existing icons in the app picker, you'd need something similarily ugly; yet it's certainly doable)

::: js/ui/appDisplay.js
@@ +1587,3 @@
     },
 
     _onStateChanged: function() {

With the additional redirection, I'd prefer an anonymous function here

@@ +1589,2 @@
     _onStateChanged: function() {
+        this.updateRunningStyle(this.app.state != Shell.AppState.STOPPED);

Make this private

(In reply to comment #9)
> Review of attachment 289806 [details] [review]:
> 
> (In reply to comment #8)
> > I guess a generalisation of this bug report would be that it is not simple to
> > locate and update existing icons
> 
> s/icons/any class that sets up signal handlers in its constructor/
> 
> That's my main gripe here - it certainly doesn't hurt in this particular case,
> but I don't really fancy this as a general rule; it would just add unnecessary
> redirections all over the place just in case an extension might want to change
> something. Extensions have a lot of power to do crazy stuff, but it doesn't
> have to be necessarily easy :-)

Yeah, that makes sense.

> ::: js/ui/appDisplay.js
> @@ +1587,3 @@
>      },
> 
>      _onStateChanged: function() {
> 
> With the additional redirection, I'd prefer an anonymous function here

I'm not sure if I get this one.
Are you saying I should remove _onStateChanged and connect the signal with an anonymous function? But that would require me to replace other calls to _onStateChanged too?

(In reply to comment #10)
> Are you saying I should remove _onStateChanged and connect the signal with an
> anonymous function? But that would require me to replace other calls to
> _onStateChanged too?

Yeah. Basically rename _onStateChanged to _updateRunningStyle and connect like
  this.app.connect('notify::state', Lang.bind(this,
      function() {
          this._updateRunningStyle();
      }

As far as I can see, it's only called in one other place (to set the initial style) ...

Created attachment 290058 [details] [review]
Proposed patch

Addressed issues.
Also, dropped the isRunning parameter.

Review of attachment 290058 [details] [review]:

Works for me - the commit message is slightly outdated now though.

Created attachment 290063 [details] [review]
Proposed patch

Updated commit message to represent the new changes.

Review of attachment 290063 [details] [review]:

Sorry, somehow missed the bugmail about the updated patch; looks good to me now!

Cool.
I don't have commit access, but I assume that I can make a request for an account as described on https://wiki.gnome.org/AccountsTeam/NewAccounts
Is that correct?

Pushed to master as 24c0a1a1d458 and gnome-3-14 as dcd3945bb7c45d.