[Sugar-devel] [PATCH sugar] SugarAdhoc networks: make 'connect' option in Palette available

[Sascha Silbe]

Excerpts from Simon Schampijer's message of 2012-01-04 13:23:46 +0100:

> The palette got only updated on device state changes so far,
> but is not initialised on startup. The device state is now signaled
> as well on bringup from the manager to the widget.

It took me a while to figure out why the first change (calling
start_listening() later) is needed, as the fix is in MeshBox, but what
fails is the interaction between AdHocManager and SugarAdhocView.

And while your patch has the effect of revealing the connect action, it
really fixes the initialisation. But maybe I'm splitting hairs here.
Anyway, this is my take on it:

=== snip ===
Fix initialisation of ad-hoc network panels (reveals connect action)

SugarAdhocView (which shows an individual ad-hoc network icon in the
Neighbourhood) defaults to an undefined state, not showing any action on
its palette.

Formerly, AdHocManager didn't propagate the state of the network device
via the 'update-state' signal when hooking to NetworkManager
(AdHocManager.start_listening()), so the palettes didn't get initialised
to a known state. The connect action on ad-hoc icon palettes only got
revealed after the state of the network device changed for some reason
(e.g. connecting to a network).

While SugarAdhocView shouldn't depend on the update-state signal getting
delivered right after instantiation, the NetworkManager client code in
Sugar needs major rework and most of the ad-hoc support should move into
NetworkManager, so we're doing a minimally invasive change instead. The
change is both correct and required in any case (as one of the ad-hoc
networks could have been connected to before the current instance of
Sugar got started).

The ad-hoc network icons (SugarAdhocView instances) now get instantiated
before AdHocManager.start_listening() is called, so they can receive the
'update-state' signal that AdHocManager.start_listening() will now
broadcast after hooking up to NetworkManager.
=== snip ===

Feel free to include as much of it as you consider useful in your commit
message.


Please remember to include your sign-off to indicate acceptance of our
patch submission guidelines, including permission to distribute your
code under the current license of the module your modifying.


[src/jarabe/desktop/meshbox.py: MeshBox]
>      def add_adhoc_networks(self, device):
>          if self._adhoc_manager is None:
>              self._adhoc_manager = get_adhoc_manager_instance()
> -        self._adhoc_manager.start_listening(device)
>          self._add_adhoc_network_icon(1)
>          self._add_adhoc_network_icon(6)
>          self._add_adhoc_network_icon(11)
> +        self._adhoc_manager.start_listening(device)
>          self._adhoc_manager.autoconnect()

I'd argue that SugarAdhocView is broken because it _relies_ on the
update-state signal getting delivered in order to complete
initialisation. Instead it should initialise itself to a default state
(i.e. being disconnected). If the network device isn't usable, the icons
should be hidden rather than being shown without any action.

But since the NetworkManager client code in Sugar is quite a mess and we
should move the ad-hoc functionality to NetworkManager anyway, your
minimally invasive change is still the way to go. We should just make
sure we know and document what we're doing.


Acked-by: Sascha Silbe <silbe at activitycentral.com>

Sascha

[1] https://wiki.sugarlabs.org/go/Development_Team/Code_Review#Patch_submission
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120116/9a17e48f/attachment.pgp>