3G (GSM) Modem Support

[tch]

Since Paraguay and Uruguay (and others) deployments has been asking for 3G modem support, I have been working on it and here is the patch

What it does?

1. Modifies jarabe.model.network, adding the GSM settings classes required to store and manage this kind of connections, and changes the connection list indexation from ssids to uuids (so i had to modify jarabe.desktop.meshbox and deviceicon.network)

2. Adds a new control panel section for basic settings information for gsm connections.

3. Adds a new device icon for gsm devices (where i also modified a few lines concerning the changes at jarabe.model.network)

TODO: Since I only have GSM modems available, its the only 3G device i added, theres still other kind of devices but the work should be similar (Adding new classes for specific settings)

Test case:

1. Go to control panel, Modem Configuration
2. Set username, password, number, and apn
3. Save changes and restart
4. Connect the device and wait until the device icon appears
5. Click on connect item
7. Click on disconnect item

Waiting for review,

[tch]

Added minor changes to the main patch, some info and code clean up at control panel view

[tomeu]

+# Copyright (C) 2009 Martin Abente

Is your employer ok with this? (Just trying to avoid possible problems in the future)

+storage = StorageGsm()

Better instantiate it when it's actually needed, see here for how we use to implement singletons:

 http://git.sugarlabs.org/projects/sugar/repos/mainline/blobs/master/src/jarabe/model/shell.py#line602

+storage = StorageGsm()

Do we really need such a thin abstraction layer? It adds an indirection but I don't see it winning us much.

+class MyEntry(gtk.Entry):

What about having UsernameEntry, PasswordEntry, etc.? Subclasses in python are very cheap. They could be subclasses of HBox, including both the label and the entry. I think that this would remove both the duplicated code and the need for setattrs/getattrs.

+ self._username_change_handler = self._username_entry.connect( \
+ 'changed', self.changed_cb)

Won't be registering one more handler every time that setup() is called? Or is it guaranteed that before every setup() is called undo()?

+ if self._username_entry._is_valid and self._password_entry._is_valid and self._number_entry._is_valid and self._apn_entry._is_valid:

This line needs to be wrapped. You could also avoid the if if you think it makes it clearer.

+_GSM_STATES = {
+ _GSM_STATE_NOT_READY : ['gsm-idle', '...', _('Please wait...')],
+ _GSM_STATE_DISCONNECTED : ['gsm-connect', _('Connect'), _('Disconnected')],
+ _GSM_STATE_CONNECTING : ['gsm-disconnect', _('Cancel'), _('Connecting...')],
+ _GSM_STATE_CONNECTED : ['gsm-disconnect', _('Disconnect'), _('Connected')]
+}

I like these constructions in general, but in this case I think we can get away easily without duplicating any code. I would set the instance variables to None in init and then call from there self.set_state(_GSM_STATE_NOT_READY) which would set them to their correct values in an ifelse statement.

+ self._current_signal = _GSM_STATES[_GSM_STATE_NOT_READY][_GSM_SIGNAL]

I would prefer if you had instead a self._current_state var and in _toggle_state_cb() you emitted one signal or the other depending on it. This is because the reader of the code would have to track where is self._current_signal assigned in order to know which signal is emitted in every case.

+ self._palette = GsmPalette()

Not really big deal, but would be better to create the palette lazily on a create_palette() method, so we reduce memory usage. Grep the code for examples.

+ logging.error('Could not connect to gsm connection, %s', detail)

Better to use logging.exception so we get a nice traceback.

+ obj = self._bus.get_object(_NM_SERVICE, _NM_PATH)
+ netmgr = dbus.Interface(obj, _NM_IFACE)
+ netmgr_props = dbus.Interface(netmgr, 'org.freedesktop.DBus.Properties')
+ active_connections_o = netmgr_props.Get(_NM_IFACE, 'ActiveConnections')

I think we should avoid using abbreviations.

+ def gsm_idle_cb(self, palette, data=None):
+ pass

What's the reason for this?

+ logging.error('Error getting gsm state %', err)

Better raising an exception so we get a backtrace.

+ return { 'gsm' : secrets }

Watch out the extra spaces.

- self.save()
+ if self._settings.connection.type == NM_CONNECTION_TYPE_802_11_WIRELESS:
+ self.save()

I think I would just add a save() method to GsmSettings that did nothing.

+ self.client = gconf.client_get_default()

Why public?

+ logging.error("While loading gsm connection from settings")

Better logging.exception and make sure that the original exception is not lost.

+ if password != :

"if password:" is enough

+ logging.error("While adding the gsm connection")

Same as before, eating exceptions make support much harder.

That's all, excellent first patch, congratulations! Hope we push it soon.

[tch]

+# Copyright (C) 2009 Martin Abente
Is your employer ok with this? (Just trying to avoid possible problems in the future)

Done

+storage = StorageGsm?()
Better instantiate it when it's actually needed, see here for how we use to implement singletons:

Done

+storage = StorageGsm?()
Do we really need such a thin abstraction layer? It adds an indirection but I don't see it winning us much.

I believe yes, first reason is that it has an small control method that makes no sence to repeat everytime i want to access this information, and second, is not recommended to spread all over the harcoded gconf paths i used for the settings, because it might change.

+class MyEntry?(gtk.Entry):
What about having UsernameEntry?, PasswordEntry?, etc.? Subclasses in python are very cheap. They could be subclasses of HBox, including both the label and the entry. I think that this would remove both the duplicated code and the need for setattrs/getattrs.

Not exactly but mostly like it, Done

+ self._username_change_handler = self._username_entry.connect( \
+ 'changed', self.changed_cb)
Won't be registering one more handler every time that setup() is called? Or is it guaranteed that before every setup() is called undo()?

It is the same logic as all other control panel sections, i can dig more about this later and report incase is a common error.

+ if self._username_entry._is_valid and self._password_entry._is_valid and self._number_entry._is_valid and self._apn_entry._is_valid:
This line needs to be wrapped. You could also avoid the if if you think it makes it clearer.

Done

+_GSM_STATES = {
+ _GSM_STATE_NOT_READY : ['gsm-idle', '...', _('Please wait...')],
+ _GSM_STATE_DISCONNECTED : ['gsm-connect', _('Connect'), _('Disconnected')],
+ _GSM_STATE_CONNECTING : ['gsm-disconnect', _('Cancel'), _('Connecting...')],
+ _GSM_STATE_CONNECTED : ['gsm-disconnect', _('Disconnect'), _('Connected')]
+}
I like these constructions in general, but in this case I think we can get away easily without duplicating any code. I would set the instance variables to None in init and then call from there self.set_state(_GSM_STATE_NOT_READY) which would set them to their correct values in an ifelse statement.

Done

+ self._current_signal = _GSM_STATES[_GSM_STATE_NOT_READY][_GSM_SIGNAL]
I would prefer if you had instead a self._current_state var and in _toggle_state_cb() you emitted one signal or the other depending on it. This is because the reader of the code would have to track where is self._current_signal assigned in order to know which signal is emitted in every case.

Done

+ self._palette = GsmPalette?()
Not really big deal, but would be better to create the palette lazily on a create_palette() method, so we reduce memory usage. Grep the code for examples.

I dont think it is necessary since GsmDeviceView is only created when its needed, so GsmPalette().

+logging.error('Could not connect to gsm connection, %s', detail)
Better to use logging.exception so we get a nice traceback.

Done

+ obj = self._bus.get_object(_NM_SERVICE, _NM_PATH)
+ netmgr = dbus.Interface(obj, _NM_IFACE)
+ netmgr_props = dbus.Interface(netmgr, 'org.freedesktop.DBus.Properties')
+ active_connections_o = netmgr_props.Get(_NM_IFACE, 'ActiveConnections?')
I think we should avoid using abbreviations.

I choose those identifiers because thats how its called in the whole code, thought it would be more readable because of that, i.e check line 463

+def gsm_idle_cb(self, palette, data=None):
+pass

To keep the same logic when the menuitem emits gsm-idle signal as any other signal, without having to disconnect and reconnect events, im open for better suggestions that doesnt change the logic for only 1 special case, :)

+ logging.error('Error getting gsm state %', err)
Better raising an exception so we get a backtrace.

Done

+ return { 'gsm' : secrets }
Watch out the extra spaces.

Done

• self.save() + if self._settings.connection.type == NM_CONNECTION_TYPE_802_11_WIRELESS: + self.save() I think I would just add a save() method to GsmSettings? that did nothing.

save() is a NMSettingsConnection() method not a Settings() method, it needs decoupling but i didnt wanted to change that part if the original code because the patch would grow even more, so i only added those control if's to avoid problems

+ self.client = gconf.client_get_default()
Why public?

Should i use a gconf.client_get_default() for every set/get?

+ logging.error("While loading gsm connection from settings")
Better logging.exception and make sure that the original exception is not lost.

Done

+if password != :
"if password:" is enough

Done

+logging.error("While adding the gsm connection")
Same as before, eating exceptions make support much harder.

Done

That's all, excellent first patch, congratulations! Hope we push it soon.

Thanks, please check the last patch to see the changes

[tomeu]

Don't forget to put the ticket again in the queue!

[tomeu]

+class ExtEntry(gtk.Entry):

Cannot get what Ext means here, I see no problem in calling it just Entry or GsmEntry or something like that.

+        self.get_method = None
+        self.set_method = None

The idea with using inheritance is that you can declare in Entry these two methods and just raise NotImplementedError, then each subclass would override it with their own implementations. Grep the code for NotImplementedError for examples.

+    def ignore_changed(self):
+        if self.changed_handler is not None:
+            self.disconnect(self.changed_handler)

It's more usual to use the block and unblock methods:  http://www.pygtk.org/docs/pygobject/class-gobject.html#method-gobject--handler-block-by-func

+        box = gtk.HBox(spacing=style.DEFAULT_SPACING)
+      
+        label = gtk.Label(entry.label_text + ':')
+        label.modify_fg(gtk.STATE_NORMAL, style.COLOR_SELECTION_GREY.get_gdk_color())
+        box.pack_start(label)

If Entry inherits from HBox and contains the label and the entry, it's easier to know from where label_text comes. Grep the code for subclasses of [HV]Box.

+        label = gtk.Label(entry.label_text + ':')

May be better that ':' is inside the translatable string, as some languages can use another symbol, or have it at the other side (RTL scripts).

+            logging.error('Could not connect to gsm connection, %s', detail)

Can you make it logging.exception so we get a nice traceback?

+    def __update_state(self, state):

Why two leading underscores?

[tomeu]

Replying to tch:

+ self._palette = GsmPalette?()
Not really big deal, but would be better to create the palette lazily on a create_palette() method, so we reduce memory usage. Grep the code for examples.

I dont think it is necessary since GsmDeviceView is only created when its needed, so GsmPalette().

GsmDeviceView will be instantiated only when needed, but right now we'll have also an instance of GsmPalette even when it is not needed. It's not only one class instance, it also beings lots of other objects, I'd say a few dozens. At some point we should move all palettes to be created lazily, given that instantiating them is fast but cost quite a bit of memory. I'm saying this because I have profiled memory usage and palettes cost a significant amount of memory.

+ self.client = gconf.client_get_default()
Why public?

Should i use a gconf.client_get_default() for every set/get?

If it's only used internally, should be named self._client. About creating an instance every time or not, I would leave it at your preference.

That's all, excellent first patch, congratulations! Hope we push it soon.

Thanks, please check the last patch to see the changes

Thanks to you, this is almost ready to go in.

[tch]

class ExtEntry(gtk.Entry):
Cannot get what Ext means here, I see no problem in calling it just Entry or GsmEntry? or something like that

Done

self.get_method = None
self.set_method = None
The idea with using inheritance is that you can declare in Entry these two methods and just raise NotImplementedError?, then each subclass would override it with their own implementations. Grep the code for NotImplementedError? for examples.

Done

def ignore_changed(self):

if self.changed_handler is not None:

self.disconnect(self.changed_handler)

It's more usual to use the block and unblock methods:  http://www.pygtk.org/docs/pygobject/class-gobject.html#method-gobject--handler-block-by-func

That is not what is supposed to do, it is not blocking and then unblocking (or viceversa) changed events, just needs to connect to it once, and then disconnect from it for good. Just as all the others control panel sections does.

box = gtk.HBox(spacing=style.DEFAULT_SPACING)
label = gtk.Label(entry.label_text + ':')
label.modify_fg(gtk.STATE_NORMAL, style.COLOR_SELECTION_GREY.get_gdk_color())
box.pack_start(label)
If Entry inherits from HBox and contains the label and the entry, it's easier to know from where label_text comes. Grep the code for subclasses of [HV]Box.

I really don't want to make the code darker, so I just wanted to have the "extended" entry class with the data needed for "control panel section" inner-control mechanisms. So now I changed the code to avoid having unnecesary stuff on this extended entry class, and keep the code readable.

label = gtk.Label(entry.label_text + ':')
May be better that ':' is inside the translatable string, as some languages can use another symbol, or have it at the other side (RTL scripts).

Done

logging.error('Could not connect to gsm connection, %s', detail)
Can you make it logging.exception so we get a nice traceback?

Done

def update_state(self, state):
Why two leading underscores?

Done

GsmDeviceView? will be instantiated only when needed, but right now we'll have also an instance of GsmPalette? even when it is not needed. It's not only one class instance, it also beings lots of other objects, I'd say a few dozens. At some point we should move all palettes to be created lazily, given that instantiating them is fast but cost quite a bit of memory. I'm saying this because I have profiled memory usage and palettes cost a significant amount of memory.

Saw the examples at meshbox.py, could someone explain me how that achieves lazyness?

If it's only used internally, should be named self._client. About creating an instance every time or not, I would leave it at your preference.

Done

Thanks to you, this is almost ready to go in.

Hopefully ;)

[tch]

Added to the gsm view class a create_palette method and re-organized the code around it

[tomeu]

A few comments:

+    # Sets text from model to entry
+    def set_text_from_model(self):
+            self.set_text(self.get_method())
+
+    # Sets text from entry to model
+    def set_text_to_model(self):
+            self.set_method(self.get_text())

Redundant comments, if you really feel the need, use docstrings. Also, watch out the indentation, here and in the rest of the patch. Consider running pylint (see wiki).

+    def get_method(self):

'method' brings nothing new, what about calling it get_value?

+    def set_method(self, username):
+        return self._model.set_username(username)

The return makes no sense.

+        self._username_entry = UsernameEntry(model)
+        username_field = self._create_field(self._username_entry, _('Username:'))
+        self.pack_start(username_field, False)

As mentioned in a previous review, it doesn't help dealing with the specifics of each entry class in two different places. Please make GsmEntry inherit from gtk.HBox and contain a label and an entry, you will be able to drop some public members, which reduces coupling and increases coherence.  http://msdn.microsoft.com/en-us/magazine/cc947917.aspx

+        self._username_entry.set_text_from_model()
+        self._password_entry.set_text_from_model()
+        self._number_entry.set_text_from_model()
+        self._apn_entry.set_text_from_model()

Why the entries don't initialize themselves in their constructors?

+GSM_USERNAME_PATH = '/sugar/nm/gsm/username'

Maybe use network instead of nm?

+class StorageGsm(object):

Still not convinced of this class being worth it.

Btw, Daniel's patch at #230 will conflict with this one.

[tomeu]

updated patch

[erikos]

Some feedback after applying the patch:

I am not sure about the work flow for this feature. I see the modem and hit 'connect'. I get no feedback when that action was not successful what went wrong or where I can set the options. How do I know I have to set some options in the CP to use the gsm-modem?

Those should go from deviceicon/network.py into jarabe.model.network

_GSM_STATE_NOT_READY = 0
_GSM_STATE_DISCONNECTED = 1
_GSM_STATE_CONNECTING = 2
_GSM_STATE_CONNECTED = 3

The items in the CP section should be left aligned like discussed in the UI meeting.

I get the following:

1264356856.836999 WARNING root: No icon with the name gsm-device was found in the theme.
1264356856.840605 DEBUG root: Connected successfully to gsm device, /org/freedesktop/NetworkManager/ActiveConnection/2
1264356856.841223 ERROR dbus.connection: Exception in handler for D-Bus signal:
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/dbus/connection.py", line 214, in maybe_handle_message
    self._handler(*args, **kwargs)
  File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 760, in __state_changed_cb
    self._update_state(int(new_state))
  File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 788, in _update_state
    self._palette.set_state(gsm_state)
  File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 237, in set_state
    self._update_label_and_text()
  File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 257, in _update_label_and_text
    'text, %s' % str(self._current_state))
ValueError: Invalid GSM state while updating label and text, None

when I try to connect without any setting in the CP section. self._current_state is None in that case.

[tomeu]

Replying to erikos:

Some feedback after applying the patch:

I am not sure about the work flow for this feature. I see the modem and hit 'connect'. I get no feedback when that action was not successful what went wrong or where I can set the options. How do I know I have to set some options in the CP to use the gsm-modem?

Some discussion is going on the mailing list about these issues. We have already patches that improve the situation, but I also think that even if the experience is sub-optimal in 0.88 we should still push this, because only affects the people that have a device plugged in.

Those should go from deviceicon/network.py into jarabe.model.network
{{{
_GSM_STATE_NOT_READY = 0
_GSM_STATE_DISCONNECTED = 1
_GSM_STATE_CONNECTING = 2
_GSM_STATE_CONNECTED = 3
}}}

Why so?

The items in the CP section should be left aligned like discussed in the UI meeting.

Agreed, but think this should go in first.

I get the following:
{{{
1264356856.836999 WARNING root: No icon with the name gsm-device was found in the theme.
1264356856.840605 DEBUG root: Connected successfully to gsm device, /org/freedesktop/NetworkManager/ActiveConnection/2
1264356856.841223 ERROR dbus.connection: Exception in handler for D-Bus signal:
Traceback (most recent call last):
File "/usr/lib/python2.6/site-packages/dbus/connection.py", line 214, in maybe_handle_message
self._handler(*args, **kwargs)
File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 760, in state_changed_cb
self._update_state(int(new_state))
File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 788, in _update_state
self._palette.set_state(gsm_state)
File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 237, in set_state
self._update_label_and_text()
File "/home/erikos/sugar-jhbuild/install/share/sugar/extensions/deviceicon/network.py", line 257, in _update_label_and_text
'text, %s' % str(self._current_state))
ValueError: Invalid GSM state while updating label and text, None
}}}
when I try to connect without any setting in the CP section. self._current_state is None in that case.

Daniel Castelo already has a patch that improves this, waiting for this one to go in.

[tomeu]

Pushed as  http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/35035af4

Thanks all for the code and feedback, we'll keep polishing this.