Just found a mailing list discussion on the previous attempt to include this in nm-applet. Pity it was never actually done.

https://mail.gnome.org/archives/networkmanager-list/2011-March/msg00030.html

Created attachment 291425 [details] [review]
0001-use-hash-table-for-icon-caching.patch

Created attachment 291426 [details] [review]
0002-return-icon-name-from-get_icon-whenever-possible.patch

Created attachment 291427 [details] [review]
0003-indicator-add-support-for-enable-indicator.patch

Created attachment 291428 [details] [review]
0004-indicator-add-device-specific-icon-code.patch

This is what I have. It's a substantially reworked version of the latest Ubuntu patch (I can't find a link right now).

First reviews welcome. This will probably take a bit longer, I expect a lot of issues and improvements.

URL of the Ubuntu package:

http://packages.ubuntu.com/source/vivid/network-manager-applet

URL of the original patch:

http://bazaar.launchpad.net/~network-manager/network-manager-applet/ubuntu/view/head:/debian/patches/nm-applet-use-indicator.patch

Posponing review. There are two things I want to change. First of all --disable-indicator won't work now, second, there are many things in the indicator patch that could go to the previous patch, actually.

Created attachment 291494 [details] [review]
0001-use-hash-table-for-icon-caching.patch

Created attachment 291495 [details] [review]
0002-remove-pos-from-applet_menu_item_add_complex_separat.patch

Created attachment 291496 [details] [review]
0003-return-icon-name-from-get_icon-whenever-possible.patch

Created attachment 291497 [details] [review]
0004-indicator-add-support-for-enable-indicator.patch

Created attachment 291498 [details] [review]
0005-indicator-add-device-specific-icon-code.patch

(In reply to comment #8)
> First of all --disable-indicator won't work now,

Fixed.

> second, there are many things in the indicator patch that could go to the previous patch, actually.

Reworked.

The resulting patchset still doesn't work great with --enable-indicator but I would still like to get reviewed and merged at least the first three patches and I'll be happy for comments on the last two as well.

>> use hash table for icon caching

in nma_icon_check_and_load():

-         return applet->fallback_icon;
+         icon = g_object_ref (applet->fallback_icon);
      }

once we fail to load the ion, add the fallback to the hash table.


Minor complaint: nma_icon_check_and_load() should have a name like "applet_icon_*()", and @applet should be the first parameter. So that it has the look of being a method of "NMApplet".

But this is already pre-existing, so feel free not to do that...



>> return icon name from get_icon() whenever possible

bond_get_icon() and related might not set the out-arguments.
Also, they require the out-args to be not NULL, while applet_get_device_icon_for_state() allows for missing arguments.

I think applet_get_device_icon_for_state() should resolve this by ensuring out-args are always set (to NULL) and passing a dummy argument to dclass->get_icon() in case the caller passed NULL.


> indicator: add support for --enable-indicator

+#ifdef ENABLE_INDICATOR
+    if (app_indicator_get_status (applet->app_indicator) == APP_INDICATOR_STATUS_PASSIVE)
+#endif
     if (!gtk_status_icon_is_embedded (applet->status_icon))

this looks wrong. Add Braces and fix indention?

(In reply to comment #15)
> >> use hash table for icon caching
> 
> in nma_icon_check_and_load():
> 
> -         return applet->fallback_icon;
> +         icon = g_object_ref (applet->fallback_icon);
>       }
> 
> once we fail to load the ion, add the fallback to the hash table.

As you wish. The original code seemed to retry each time but I agree that it's not useful.

> Minor complaint: nma_icon_check_and_load() should have a name like
> "applet_icon_*()",

I definitely agree, I didn't want to change the name without feedback from the upstream, though. Let's agree on some name. I propose `applet_icon()` or `applet_get_icon()`. The API is there to retrieve an icon by name, the checking, loading and caching are implementation details.

> and @applet should be the first parameter. So that it has
> the look of being a method of "NMApplet".

I'll handle this once we have a final name.

> But this is already pre-existing, so feel free not to do that...

I'll be happy to improve nm-applet as you instruct ;).

> >> return icon name from get_icon() whenever possible
> 
> bond_get_icon() and related might not set the out-arguments.

The purpose is to keep all common code in `applet_get_device_icon_for_state()`. Let's talk about that function instead.

> Also, they require the out-args to be not NULL, while
> applet_get_device_icon_for_state() allows for missing arguments.

I was actually trying to make it effective. We might want to talk on IRC on how exactly this should be improved.

> I think applet_get_device_icon_for_state() should resolve this by ensuring
> out-args are always set (to NULL) and passing a dummy argument to
> dclass->get_icon() in case the caller passed NULL.

Basically yes, let's talk about the details off bugzilla.

> > indicator: add support for --enable-indicator
> 
> +#ifdef ENABLE_INDICATOR
> +    if (app_indicator_get_status (applet->app_indicator) ==
> APP_INDICATOR_STATUS_PASSIVE)
> +#endif
>      if (!gtk_status_icon_is_embedded (applet->status_icon))
> 
> this looks wrong. Add Braces and fix indention?

It's the easiest way to write it. With braces, you will have two ifdefs or you will have to duplicate the second if. Feel free to decide on the preferred way.

I'm happy with "applet_get_icon()".  That can be a follow-up patch though.

I think the original code retried each time to ensure it picked up new icons on theme changes, and to ensure that we didn't get broken icons in the cache forever because something (package manager) was writing new ones to disk when a theme change happened.  Or something like that.  Happy to drop that part now since this seems unlikely.

Also, if you wouldn't mind updating the commit message shortlog with the "applet: " prefix that would be great.

The first two are uncontroversial, I have pushed them:

9e037ec34ee6efabbdb7cede0a5b95d995a602bb
applet: use hash table for icon caching

995b6800661948c4f6760641effdee924f40c440
applet: remove pos from applet_menu_item_add_complex_separator_helper()

The third (return icon name from get_icon() whenever possible) can probably be pushed very soon too, after some small fixups for thaller's comments.

I'd take a patch to rename nma_icon_check_and_load() too.

Comment on attachment 291495 [details] [review]
0002-remove-pos-from-applet_menu_item_add_complex_separat.patch

(pushed)

I'm sorry for delaying this, I made some heavy modifications and only then I realized that it's too much and was too tired already. So I restarted with just the minimal changes to get the patch in a shape...

Created attachment 292336 [details] [review]
this is what I changed

Created attachment 292337 [details] [review]
0001-return-icon-name-from-get_icon-whenever-possible.patch

And this is the new patch.

Created attachment 292338 [details] [review]
0001-return-icon-name-from-get_icon-whenever-possible.patch

Another improved version. We now rely on nm_icon_check_and_load() always returing a pixbuf or aborting. That's a reasonable requirement as that function already returns a fallback icon where appropriate and it simplifies the code substantially.

Also pushed the whole branch to:

https://github.com/pavlix/nm-applet

Review of attachment 292338 [details] [review]:

::: src/applet.c
@@ +2392,3 @@
+		case ICON_LAYER_VPN:
+			tip = g_string_new (applet->tip);
+

This seems a bit fragile, to use applet->tip to build the VPN bits.  Yeah, I know it gets set to the right thing by the first call of this function in applet_update_icon().  But it's depending on a side-effect of a previous call, and I think we can clean that up more.

How about instead move the tip code back to applet_update_icon(), but make it more explicit?  I'll attach a patch to show what I mean; I think it'll be a lot simpler.

@@ +2450,3 @@
+#else
+	gtk_status_icon_set_tooltip (applet->status_icon, applet->tip);
+#endif

The applet requires GTK3+ now, so we can just call tooltip_text()

@@ +2897,2 @@
 out:
+	applet_common_get_device_icon (state, out_pixbuf, out_icon_name, applet);

Shouldn't t here be a return or something in the 'if (dclass)' bits?  If not, then applet_common_get_device_icon() could overwrite the allocated values from the dclass->get_icon() bits.

@@ +3024,3 @@
+	foo_set_icon (applet, ICON_LAYER_VPN, pixbuf, icon_name, vpn_tip);
+	if (icon_name)
+		g_free (icon_name);

See my fixup patch for what I think is a simpler way to handle foo_set_icon() and the tooltips.

Created attachment 292625 [details] [review]
0001-fixup-return-icon-name-from-get_icon-whenever-possib.patch

A fixup patch for 0001-return-icon-name-from-get_icon-whenever-possible.patch that simplifies tooltip handling.

Are we ready to push if the fixup is applied?

(In reply to comment #28)
> Are we ready to push if the fixup is applied?

The fixup doesn't address this comment:

@@ +2897,2 @@
 out:
+    applet_common_get_device_icon (state, out_pixbuf, out_icon_name, applet);

Shouldn't t here be a return or something in the 'if (dclass)' bits?  If not,
then applet_common_get_device_icon() could overwrite the allocated values from
the dclass->get_icon() bits.

So that piece is still outstanding.  Once that's fixed up I think we can push.  If you can do that today/tomorrow then I think it can go in for 1.0.

Created attachment 292861 [details] [review]
0001-return-icon-name-from-get_icon-whenever-possible.patch

fixed and tested version of the first patch

Created attachment 292862 [details] [review]
0002-indicator-add-support-for-enable-indicator.patch

No change (apart from rebasing), just fixing the order.

Created attachment 292863 [details] [review]
0003-indicator-add-device-specific-icon-code.patch

Removed unintended change for the non-indicator mode.

Basically patches 0002 and 0003 don't affect indicator mode (unless I missed something else). The `applet_schedule_update_menu()` is a noop in non-indicator mode. Other changes are in #ifdef sections.

Created attachment 292883 [details] [review]
fixup for attachment  292861 [details] [review]

Created attachment 292884 [details]
fixup for attachment   292862 [details] [review]

Created attachment 292885 [details] [review]
fixup for attachment  292863 [details] [review]

Created attachment 292892 [details] [review]
fixup for attachment 292862 [details] [review]

Attached 4 minor patches with fixups.



setup_widgets():

I pushed a fixup for this (with no functional change). But is it correct, that we execute *both* setup functions and then
  "return success || success_setup_indicator" ?

Maybe better:

+#ifdef ENABLE_INDICATOR
+    if (!setup_indicator_menu (applet))
+         return FALSE;
+#endif
+
+    return success;



I did not carefully review new parts with "#ifdef ENABLE_INDICATOR". I think it's new code, and looks good (certainly good enough for inclusion). If there happen to be issues, let's fix them later.

I cared more about changes to pre-existing code, which gets also fixed in the same patch. Would be nice to have them separate, because they seem to fix preexisting issues that might be worth backporting separately. Anyway, just talking generally. Don't bother now. I'm ok with how it is.



but for:
applet_connection_info_cb():
     menu = GTK_MENU_SHELL (gtk_menu_new ());
+    g_object_ref_sink (menu);
 

now we take an additional ref. Is this correct?



Overall, LGTM!

(In reply to comment #37)
> Attached 4 minor patches with fixups.
> 
> 
> 
> setup_widgets():
> 
> I pushed a fixup for this (with no functional change). But is it correct, that
> we execute *both* setup functions and then
>   "return success || success_setup_indicator" ?

It doesn't sound correct, I will look at it.

> I did not carefully review new parts with "#ifdef ENABLE_INDICATOR". I think
> it's new code, and looks good (certainly good enough for inclusion). If there
> happen to be issues, let's fix them later.

+1

> I cared more about changes to pre-existing code, which gets also fixed in the
> same patch. Would be nice to have them separate, because they seem to fix
> preexisting issues that might be worth backporting separately.

I will try to split the patches, accordingly just after lunch.

> but for:
> applet_connection_info_cb():
>      menu = GTK_MENU_SHELL (gtk_menu_new ());
> +    g_object_ref_sink (menu);
> 
> 
> now we take an additional ref. Is this correct?

Probably not. Removed.

Current version available at:

Repository: git@github.com:pavlix/nm-applet.git
Branch: indicator

Updated branch, split patches, improved setup_widgets. The appindicator part currently fails, will fixup the respective patch ASAP.

Updated branch, now nma_icons_init() is called only for non-indicator mode. Icon and menu is now shown also when built with libappindicator.

>> trivial: use assert, not g_return_if_fail

usually I prefer g_return_*() over g_assert(). The former *is* effectively an assertion and must be fixed just the same. But it's a bit more forgiving in face of a bug. With this change, nma will dump core, while otherwise it might be able to limp away.

I would drop this patch.

(also, I would not call it "trivial")




I like all the other patches, now it's more easy to see that they add new code and leave existing code untouched.

(In reply to comment #41)
> >> trivial: use assert, not g_return_if_fail
> 
> usually I prefer g_return_*() over g_assert()

All of those are about the existence of the main object. It always exists. I wouldn't be afraid of dropping the assertions entirely. Using `g_return_*()` suggests that the caller should check the result for being NULL which means additional code that has no real function.

Feel free to drop the patch or keep it as you wish, though.

> I like all the other patches, now it's more easy to see that they add new code
> and leave existing code untouched.

Thanks! I don't have push access, so please get it pushed.

(In reply to comment #38)
> Repository: git@github.com:pavlix/nm-applet.git
> Branch: indicator

Removed the assertion patch.

Issues were found with nm-applet initialization, I will give it a bit more love and come back with an updated patchset.

> indicator: add device specific icon code

I'd actually rather just use the same code for indicator & non-indicator cases.  libdbusmenu removes pango markup, so we don't  need to bother stripping that out ourselves.

Second, GtkImageMenuItem is deprecated, so we should be using GtkMenuItem (which is a Bin) and packing an Box + Label + Image into the menu item instead.  However, libdbusmenu upstream doesn't support images like this, but only supports GtkImageMenuItem (which of course is deprecated).  libdbusmenu upstream should be updated to ensure that images can be used in normal GtkMenuItems.

I'm working on some adjustments to the menu itself, but could you provide a picture of what the appindicator version looks like when the icons show up?

By the way you can easily test the indicator code by building it with the correct parameters and running it from the source directory. If you don't have a system tray that supports it, you can use stalonetray.

    ./autogen.sh --prefix=/usr --sysconfdir=/etc --enable-indicator
    ./src/nm-applet

(In reply to comment #45)
> > indicator: add device specific icon code
> 
> I'd actually rather just use the same code for indicator & non-indicator cases.

Same here.

Just improving the patchset in small steps until it can get accepted and then would continue to improve the code further.

>  libdbusmenu removes pango markup, so we don't  need to bother stripping that
> out ourselves.

Any fixups?

> Second, GtkImageMenuItem is deprecated, so we should be using GtkMenuItem
> (which is a Bin) and packing an Box + Label + Image into the menu item instead.
>  However, libdbusmenu upstream doesn't support images like this, but only
> supports GtkImageMenuItem (which of course is deprecated).  libdbusmenu
> upstream should be updated to ensure that images can be used in normal
> GtkMenuItems.

For now I would live with what is there and just use a couple of ifdefs to keep support for libdbusmenu. I don't see a good reason to delay delivering the feature at least into the tree because of such minor issues.

So what is the plan?

(In reply to comment #43)
> (In reply to comment #38)
> > Repository: git@github.com:pavlix/nm-applet.git
> > Branch: indicator

New patchset pushed. Removed almost all warnings.

From my debugging it is clear that the conflicting registration comes from the same nm-applet instance.

How to test:

CFLAGS=-ggdb ./autogen.sh --prefix=/usr --sysconfdir=/etc --disable-migration --enable-indicator
make
./src/nm-applet --no-agent

Note: I'm using --disable-migration and --no-agent to avoid other warnings.

There is still the warning caused by repeated call to app_indicator_set_menu():

(nm-applet:15546): libappindicator-WARNING **: Unable to connect to the Notification Watcher: GDBus.Error:org.kde.StatusNotifierWatcher.Item.AlreadyRegistered

Trace back for the warning:

+ Trace 234553

The systray implementation is Enlightenment git master. As I see it, the problem is either that libappindicator registers again when app_indicator_set_menu() gets called the second time, or Enlightenment doesn't handle the update.

Pushed an updated version, fixed non-indicator version, removed stuff that cause bug #744150. Will test in indicator mode later.

Added further fixes, split out some patches, removed some unrelated changes, changed --enable-indicatir to --with-appindicator (as in build with the libappindicator library). Tested appindicator mode a bit. 

af13a322ab31d575d5ff566ed96b9b9a00d6a130 applet: don't unref menu item widgets
a32f62746ad2ccf0848da50e6f1e2d30fd6702f6 applet: initialize nm-client before the widgets
ac0ac243fe1bdad06225e0f91cf43fd38fb636dd applet: simplify initialization and finalization
d53343d85400716364ffd89e3375e6914f31d8bc applet: return icon name from from applet_update_icon()
6e6c47c38e4711123d1e615e84e2f41535bd2570 applet: prepare for appindicator patch
45aed6b41223554b89963bb16c05bf6edfcffcab applet: add support for --with-appindicator

https://github.com/pavlix/nm-applet/commits/indicator

I'm starting to use the resulting code on a daily basis and it seems to work well. Please review and push to git master.

(In reply to comment #50)
> Please review and push to git master.

Except the last EXPERIMENTAL patch of course.

Some screenshots (enlightenment + nm-applet):

http://www.enlightenment.org/ss/e-54d7711cf392c9.17663480.jpg
http://www.enlightenment.org/ss/e-54d77194b036b3.97183338.jpg

Note there's still a warning (only for --with-appindicator) but I don't think it's caused by nm-applet.

(nm-applet:16980): libappindicator-WARNING **: Unable to connect to the Notification Watcher: GDBus.Error:org.kde.StatusNotifierWatcher.Item.AlreadyRegistered

Bump.

I pushed the first three patches since they are uncontroversial:

applet: don't unref menu item widgets
applet: initialize nm-client before the widgets
applet: simplify initialization and finalization

Thanks! Pushed a rebased version version (on github). On IRC we talked about the necessity to pass icon names instead of pixbufs. We also talked about using multiple stock icons for menu items where it makes sense.

We concluded that the resulting code is not perfect and that nm-applet appindicator support needs continued work. We didn't come to a conclusion as whether the continued work will happen after merging the rest of the patches or whether the patches should be modified.

You also pushed fixups to master. I'm still urging to get at least EXPERIMENTAL level of libappindicator support landed to git master ASAP. I promised to continue providing fixup/improvement patches after the appindicator support is in master.

Everyone: Please continue reviewing and testing!

Ok, please look at dcbw/indicator and see if the many modifications to "applet: add support for --with-appindicator" work for you.  In summary, they are:

1) move all indicator-specific stuff out of applet-device-* and into mb-menu-item.c and ap-menu-item.c

2) bring back composited icons in the menu, since dbusmenu and libappindicator supports this (tested in GNOME Shell with the appindicator extension, and yes they work)

3) removal of CFLAGS/LIBADD stuff from the gconf-helpers code, no reason they should be needed there

4) make the VPN submenu items always check-menu-items instead of ImageMenuItems, like the indicator version already does

5) remove some #ifdefs around gtk_widget_show() which shouldn't hurt the non-indicator case

What I wasn't really happy about, and apparently didn't express clearly enough on IRC, was the half-usage of the ap-menu-item.c and mb-menu-item.c stuff.  I wanted them to be used the same way from applet-device-* regardless of whether indicator was enabled or not, so that the menu objects encapsulated the differences of indicator vs. non-indicator, leaving much cleaner code.  The patch is now like net +200 LoC instead of twice that.

What do you think?  Can you test this branch and let me know if it works for you?

With dcbw/indicator:

CFLAGS=-ggdb ./autogen.sh --prefix=/usr --sysconfdir=/etc --disable-migration --with-appindicator && make && ./src/nm-applet

https://www.enlightenment.org/ss/e-54e33054e5e694.48406175.jpg

Created attachment 297023 [details]
Screenshot of dcbw/indicator with GNOME Shell appindicator extension

> 8beedc3e565b343aeb36aed96741044040011c3d
> applet: simplify mobile broadband menu item finalization

+1 (no real relation to appindicator patchset)

> 0c3600238bb2da31af48d7184df5cf3ddebec2b8
> applet: clean up NMNetworkMenuItem

+1 (no real relation to appindicator patchset)

> 6f75bfefbd6c0605666191d5153b8a83974a3f1c
> applet: remove obsolete support for ModemManager <= 0.7

+1 (no real relation to appindicator patchset)

> 7f45ba6c25a8fe6a94b55b68d6ad20110f288492
> applet: return icon name from from applet_update_icon()

+1 (one of appindicator patches)

> 40f052d38af9e9f3bc42c6cbbb51e9044ab73663
> applet: prepare for appindicator patch

This one really belongs either just before the final patch or squashed into the final patch. I'll keep it separate for now. I also see some bad changes there that haven't been part of the patchset, so I'm afraid you used an older version of the patchset.

> d558ea0c58c1adfd3d11d6c2a5bc960183347b13
> applet: simplify NMNetworkMenuItem initialization

Great job but I moved it before the previous patch.

> bb6e147576ca7679ac82c50fba57f1d44da7d267 applet: add support for --with-appindicator

Good, except that it wasn't meant ot contain changes to the non-appindicator code. I moved those changes into the "prepare" patch and split that one into smaller pieces.

Just pushed a branch:

8beedc3e565b343aeb36aed96741044040011c3d applet: simplify mobile broadband menu item finalization
0c3600238bb2da31af48d7184df5cf3ddebec2b8 applet: clean up NMNetworkMenuItem
6f75bfefbd6c0605666191d5153b8a83974a3f1c applet: remove obsolete support for ModemManager <= 0.7
7f45ba6c25a8fe6a94b55b68d6ad20110f288492 applet: return icon name from from applet_update_icon()
dc3d725b4b6797697757a65c3a1f7123b5a5c736 applet: simplify NMNetworkMenuItem initialization
9a7bd6b2deb8d09c3dfb519e72d3fe468c246a77 applet: clean up menu item label handling
d9f0a1f1c440d32adf4a0637c35462d203dd1513 applet: add applet_schedule_update_menu()
b15b48ff0890f2b51288ad98724e0a8d0ce12bf1 applet: run gtk_widget_show()
bbe45f583f372ed486f2255f559aa81d26bade53 applet: don't call gtk_image_new_from_stock()
37f62b6524a07f091cb68a1189a2922ced3795e9 applet: use GtkCheckMenuItem
e14bada7d5c253993b3de9595e041df122b8249e applet: wait with hash table lookup after assertions
751b6fcc236b7157acf7721cfbe381e952dd142e applet: simplify nma_icons_init()
c5b1eca82c6808afd3a6668ac0b0e9ae4c378c6c applet: prepare for appindicator patch
2bd4e529cd09133914f0af93df67f372a10d9300 applet: add support for --with-appindicator

Most of the patches should be good to go. The final result uses pixbuf but those seem to be not supported by Enlightenment, I'll investigate it a bit more.

(In reply to Pavel Simerda from comment #57)
> With dcbw/indicator:
> 
> CFLAGS=-ggdb ./autogen.sh --prefix=/usr --sysconfdir=/etc
> --disable-migration --with-appindicator && make && ./src/nm-applet
> 
> https://www.enlightenment.org/ss/e-54e33054e5e694.48406175.jpg

This turned out to be the screenshot of pavlix/indicator, not dcbw/indicator with which I have no menu icons at all.

Thanks for pushing part of the patches during IRC discussion and for preparing the EXPERIMENTAL patch. The current indicator branch on my github is ready to merge into master except EXPERIMENTAL.

Branch pushed to master with a small fixup to the GtkCheckMenuItem patch that removes an orphaned call to gtk_image_menu_item_set_image().

Also pushed 59678db1c7e0b3afbad15356b6936b614dd61cd2 (applet/indicator: use icon names by default) which defaults to icon names.  Please test and let me know if this all works.

The release tracker can be used to find (some of the) features that have been added before that release. Adding back.

Build nm-applet master and started using it from now on. Works great with Enlightenment git master. There is a known regression from non-appindicator mode of not showing the composite pixmaps (nor the respective information) for the status icon and for wifi menu items.

As my work was inspiried by using Enlightenment, the next step would be to contact them about the composite pixbufs. Then we will probably want to make a choice, either automatic or manual, whether to use icon names or pixbufs. One idea is to actually default to pixbufs (as it's fuller in features) and switch to icon names via 'nm-applet --no-pixbufs'.

Anyway, the worst part is behind us, thank you! The danger of having huge incompatible patches in distributions is over.

(git master)

http://www.enlightenment.org/ss/e-54e45f614f3179.19611612.jpg
http://www.enlightenment.org/ss/e-54e45f949523d8.51047365.jpg

I wonder, why is this BZ still open? What is left to do?


(In reply to Pavel Simerda from comment #63)
> Build nm-applet master and started using it from now on. Works great with
> Enlightenment git master. There is a known regression from non-appindicator
> mode of not showing the composite pixmaps (nor the respective information)
> for the status icon and for wifi menu items.

"Non-composite", you mean that for example there is no longer an indication in the icon for encrypted Wi-Fis, right?

Is that unfixable, or just to be done?

> As my work was inspiried by using Enlightenment, the next step would be to
> contact them about the composite pixbufs. Then we will probably want to make
> a choice, either automatic or manual, whether to use icon names or pixbufs.
> One idea is to actually default to pixbufs (as it's fuller in features) and
> switch to icon names via 'nm-applet --no-pixbufs'.



I also noticed, that for me (plasma5), nm-applet no longer reacts on left-mouse click. And on right-mouse-click, the popup menue is a combination of what used to be left+right. Is that also correct? Is it a known issue?

(In reply to Thomas Haller from comment #65)
> I wonder, why is this BZ still open? What is left to do?
> 
> 
> (In reply to Pavel Simerda from comment #63)
> > Build nm-applet master and started using it from now on. Works great with
> > Enlightenment git master. There is a known regression from non-appindicator
> > mode of not showing the composite pixmaps (nor the respective information)
> > for the status icon and for wifi menu items.
> 
> "Non-composite", you mean that for example there is no longer an indication
> in the icon for encrypted Wi-Fis, right?

It's fixable in two ways:

1) we create new icons for every possible permutation of the final icon

2) somebody enhances dbusmenu/appindicator to allow sending the pixmap data instead of just an icon name.  eg, see the code protected by DBUSMENU_PIXMAP_SUPPORT.

> Is that unfixable, or just to be done?
> 
> > As my work was inspiried by using Enlightenment, the next step would be to
> > contact them about the composite pixbufs. Then we will probably want to make
> > a choice, either automatic or manual, whether to use icon names or pixbufs.
> > One idea is to actually default to pixbufs (as it's fuller in features) and
> > switch to icon names via 'nm-applet --no-pixbufs'.
> 
> 
> 
> I also noticed, that for me (plasma5), nm-applet no longer reacts on
> left-mouse click. And on right-mouse-click, the popup menue is a combination
> of what used to be left+right. Is that also correct? Is it a known issue?

I'm not sure about left-click vs. right-click thing, but the "combined menu" thing is definitely intentional because I don't htink dbusmenu/appindicator allows the icon to differentiate left and right click.  You just get the the "show yourself" signal.  So all of our info has to be in one menu.

-----

The last thing that would be great, and hugely clean up the applet implementation would be if dbusmenu/appindictor could handle composed GtkMenuItem objects.  GtkImageMenuItem is deprecated upstream, but we have to use it here because that's the only way that the GTK dbusmenu bindings accept an icon.  If the GTK dbusmenu bindings could be updated to look at the children of the GtkMenuItem for a GtkImage object and a GtkLabel object, it should work for both composed GtkMenuItems and existing GtkImageMenuItems.  Then we could remove a lot of the #ifdef stuff from the applet.

https://code.launchpad.net/~dcbw/libdbusmenu/libdbusmenu/+merge/286843

once that's applied to libdbusmenu we can merge this and depend on an up-to-date libdbusmenu:

dcbw/indicator-cleanups

and the only ENABLE_INDICATOR ifdefs are in applet.c.

The libdbusmenu and dcbw/indicator-cleanups do a few things:

1) dbusmenu can already handle Pango markup, so just remove that ifdef stuff.

2) the dbusmenu changes look for GtkImages in regular GtkMenuItems which means we can use our composited pixmaps (like security lock over wifi strength) in the icon

3) which means we can remove usage of the deprecated GtkImageMenuItem, which we were only keeping around to enable the appindicator stuff

+    static gboolean icons_shown = FALSE;
 
     g_return_val_if_fail (applet != NULL, NULL);
 
     menu = GTK_MENU_SHELL (gtk_menu_new ());
 
+    if (G_UNLIKELY (icons_shown == FALSE)) {
+         GtkSettings *settings = gtk_widget_get_settings (GTK_WIDGET (menu));
+
+         /* We always want our icons displayed */
+         if (settings)
+              g_object_set (G_OBJECT (settings), "gtk-menu-images", TRUE, NULL);
+         icons_shown = TRUE;
+    }


this seems very strange. I don't understand, why we set the property of a newly created @menu only once (guarded by a static variable).




The rest lgtm.




If these patches require a very new libdbusmenu, maybe we should hold them back? At least the last one...

(In reply to Thomas Haller from comment #69)
> 
> 
> +    static gboolean icons_shown = FALSE;
>  
>      g_return_val_if_fail (applet != NULL, NULL);
>  
>      menu = GTK_MENU_SHELL (gtk_menu_new ());
>  
> +    if (G_UNLIKELY (icons_shown == FALSE)) {
> +         GtkSettings *settings = gtk_widget_get_settings (GTK_WIDGET
> (menu));
> +
> +         /* We always want our icons displayed */
> +         if (settings)
> +              g_object_set (G_OBJECT (settings), "gtk-menu-images", TRUE,
> NULL);
> +         icons_shown = TRUE;
> +    }
> 
> 
> this seems very strange. I don't understand, why we set the property of a
> newly created @menu only once (guarded by a static variable).

GtkSettings are actually a global object for the GdkScreen this widget is displayed on (and in this case, we only care about one screen), so we only need to set the property once.  Hence why it's guarded with a static variable.

We set this property to TRUE because libdbusmenu checks this property to see whether or not to display the icon that we have packed into our GtkMenuItem, like the wifi strength icon or the WWAN signal icon.  If the gtk-menu-images property is not TRUE then our icons don't get displayed.  And now, most desktop environments turn menu icons off by default, so we must manually set it to TRUE.  This is also why I removed the GtkStockIcon stuff from the "Connection Information" and "About..." menu items, because by setting this property to TRUE those icons will be displayed as well.  But since standard menu icons seem to be out-of-favor these days, we don't need them anyway.

> If these patches require a very new libdbusmenu, maybe we should hold them
> back? At least the last one...

Yeah, probably.  My patches/cleanups have been merged to libdbusmenu in Launchpad, but no release yet.  I've pinged the maintainer to see if they'll do one soon.

There is now a new release of libdbusmenu with the required changes: https://launchpad.net/libdbusmenu/16.04/16.04.0

Rebased on master and updated with a pkg-config check for the necessary dbusmenu version.

Please review dcbw/indicator-cleanups

lgtm.

Maybe merge the first two patches, but wait with the last patch after 1.2? 

Merging it would mean, applet --with-appindicator won't even build on fc23, which seems a bit drastic.

I would say we just merge the whole thing; indicator is new with 1.2 and we might as well just require the dep.  We can certainly update libdbusmenu in Fedora at least, other distros can update if they want the indicator.

Repushed dcbw/indicator-cleanups with patches to allow indicator/status-icon to be built together and switched at runtime with --indicator.  Please review!

(In reply to Dan Williams from comment #75)
> Repushed dcbw/indicator-cleanups with patches to allow indicator/status-icon
> to be built together and switched at runtime with --indicator.  Please
> review!


>> applet: build (but disable) notifications code for indicator too

+#ifdef ENABLE_INDICATOR
+#define INDICATOR_ENABLED(a) (a->app_indicator != NULL)
+#else
+#define INDICATOR_ENABLED(a) (FALSE)
+#endif

parentheses around "a"?


>> applet: allow indicator and status icon to be built together
>> applet: add --indicator to switch between indicator and status icon at runtime

pushed to commits to fix building --without-appindicator.



Branch LGTM.

Created attachment 324081 [details]
screenshot of missing connection connection

with-appindicator, the connection name is missing. Also, when I press "Disconnect", there is no way to reconnect, because not connections are shown.

-    AC_DEFINE([ENABLE_INDICATOR], 1, [Enable using libappindicator])
+    AC_DEFINE([WITH_INDICATOR], 1, [Enable using libappindicator])

I think it should be "WITH_APPINDICATOR"

Squashed and repushed; how about now?

lgtm

merged to git master