> On 2010-05-26 22:03:54, Will Stephenson wrote: > > Generally fine, but udi() is not necessary, i18nc() should be used and some accessors are not marked const.
udi is necessary because uni() returns a NetworkManager id (/org/freedesktop/NetworkManager/Devices/0) and I need a ModemManager id (/org/freedesktop/ModemManager/Modems/3), which is returned by udi(). I will reimplement everything, the current implementation is too bound to GsmNeworkInterface, ModemManager can be used standalone and supports Cdma and Serial modems too, so I will reimplement this patch as a solid backend. The change is huge and today I have got some other work to do from my employer that is going to finish only in August. > On 2010-05-26 22:03:54, Will Stephenson wrote: > > /trunk/KDE/kdebase/workspace/libs/solid/control/networkgsminterface.h, lines 27-28 > > <http://reviewboard.kde.org/r/3769/diff/6/?file=27196#file27196line27> > > > > Don't use 'using' in headers; fully qualify names instead Ok. > On 2010-05-26 22:03:54, Will Stephenson wrote: > > /trunk/KDE/kdebase/workspace/libs/solid/control/networkgsminterface.cpp, lines 150-163 > > <http://reviewboard.kde.org/r/3769/diff/6/?file=27197#file27197line150> > > > > Use i18nc(comment, string-to-translate) to give context to your translations. Ok. > On 2010-05-26 22:03:54, Will Stephenson wrote: > > /trunk/KDE/kdebase/workspace/libs/solid/control/networkinterface.h, line 138 > > <http://reviewboard.kde.org/r/3769/diff/6/?file=27198#file27198line138> > > > > udi() is not necessary; uni() represents it in the Solid::Control API. udi() is necessary for ModemManager, uni() returns something like /org/freedesktop/NetworkManager/Devices/0 udi() returns something like /org/freedesktop/ModemManager/Modems/18 (the final number differs). A little bit off topic: in kdereview/networkmanagement/libs/internals/uiutils.cpp I had to change the code in UiUtils::interfaceNameLabel to use the interface name (eth0, eth1, usb0, etc) to search for a Solid::Device object to make UiUtils::interfaceNameLabel work as expected. Passing uni() as parameter to the Solid::Device constructor always returns a null/invalid Solid::Device. > On 2010-05-26 22:03:54, Will Stephenson wrote: > > /trunk/KDE/kdebase/workspace/libs/solid/control/modemmanager.h, lines 70-75 > > <http://reviewboard.kde.org/r/3769/diff/6/?file=27193#file27193line70> > > > > These enums should be written in camel case so that they match KDE/Qt coding styles and don't conflict with enums from a future ModemManager.h Ok, I am using the specification enums to find them easily inside the source code. There are several files to change to implement one single method/signal and having to translate the enums to something else all the time would make lose time. When everything is finsished I am going to change them. > On 2010-05-26 22:03:54, Will Stephenson wrote: > > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkgsminterface.h, line 54 > > <http://reviewboard.kde.org/r/3769/diff/6/?file=27216#file27216line54> > > > > Why did you make this change? Because accoding to NM-0.7 specification although Gsm devices have a propertiesChanged signal they do not have any properties, so this seemed useless to me. On the other hand ModemManager also emmits a propertiesChanged signal for Gsm devices with usefull information AllowedMode (Any, 2G, 3G, etc) and AccessTechnology (GPRS, Edge, HDSDPA, etc). Ok, that breaks the specification, I can bring it back. - Lamarque ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3769/#review5877 ----------------------------------------------------------- On 2010-05-20 01:01:06, Lamarque Souza wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/3769/ > ----------------------------------------------------------- > > (Updated 2010-05-20 01:01:06) > > > Review request for Network Management and KNetworkManager. > > > Summary > ------- > > This patch adds preliminary ModemManager support to solid. The dbus methods can be tested in command line with: > > dbus-send --system --print-reply --dest=org.freedesktop.ModemManager /org/freedesktop/ModemManager org.freedesktop.ModemManager.EnumerateDevices > > dbus-send --system --print-reply --dest=org.freedesktop.ModemManager /org/freedesktop/ModemManager/Modems/<modem number> org.freedesktop.ModemManager.Modem.Gsm.Network.GetSignalQuality > > dbus-send --system --print-reply --dest=org.freedesktop.ModemManager /org/freedesktop/ModemManager/Modems/<modem number> org.freedesktop.ModemManager.Modem.Gsm.Network.GetRegistrationInfo > > dbus-send --system --print-reply --dest=org.freedesktop.ModemManager /org/freedesktop/ModemManager/Modems/<modem number> org.freedesktop.DBus.Properties.GetAll string:<interface name> > > <modem number> is returned by the first method (EnumerateDevices). > <interface name> is either of > > org.freedesktop.ModemManager (do not use /Modems/<modem number> with this one) > org.freedesktop.DBus.Properties (do not use /Modems/<modem number> with this one) > org.freedesktop.ModemManager.Modem > org.freedesktop.ModemManager.Modem.Simple > org.freedesktop.ModemManager.Modem.Location > org.freedesktop.ModemManager.Modem.Cdma > org.freedesktop.ModemManager.Modem.Gsm > org.freedesktop.ModemManager.Modem.Gsm.Card > org.freedesktop.ModemManager.Modem.Gsm.Contacts > org.freedesktop.ModemManager.Modem.Gsm.Network > org.freedesktop.ModemManager.Modem.Gsm.SMS > org.freedesktop.ModemManager.Modem.Gsm.Hso > org.freedesktop.ModemManager.Modem.Gsm.Ussd > > There are still some problems with this patch that and I need advice as to the best way to solve them: > > 1. [FIXED] It only works for one 3G modem. NetworkManager and ModemManager have their own list of devices, I need to match the NetworkManager device id to the ModemManager to that I can use /org/freedesktop/ModemManager/Modems/<modem number> for more than one modem. For now the implementation uses the first modem found. > > 2. The ModemManager object is in NMGsmNetworkInterfacePrivate class. Maybe there is a better place to put it so other device types (CDMA, serial) could make use of ModemManager. > > 3. The patch has only been tested with my Sony Ericsson MD300 modem. > > 4. When I hook up a second 3G modem (my cell phone actually) in my notebook ModemManager disconnects the first modem from Internet. This is not a KDE problem, just to warn everyone that tries this patch. > > ModemManager dbus specification is in here http://projects.gnome.org/NetworkManager/developers/mm-spec-04.html in case someone wants to help me implement the other methods. ModemManager-0.3 does not implement all methods/signal from that specification. I am doing tests with git NetworkManager and ModemManager as of May 16, 2010. To verify which interfaces ModemManager and your modem supports do: > > dbus-send --system --print-reply --dest=org.freedesktop.ModemManager /org/freedesktop/ModemManager org.freedesktop.DBus.Introspectable.Introspect > > dbus-send --system --print-reply --dest=org.freedesktop.ModemManager /org/freedesktop/ModemManager/Modems/<modem number> org.freedesktop.DBus.Introspectable.Introspect > > Current implemenation status is > > org.freedesktop.ModemManager > - fully implemented > - still needs testing: signals DeviceAdded and DeviceRemoved > > org.freedesktop.DBus.Properties > - fully implemented and tested > > org.freedesktop.ModemManager.Modem > - modem MD300 does not supports: method GetIP4Config > > org.freedesktop.ModemManager.Modem.Simple > - to be implemented > > org.freedesktop.ModemManager.Modem.Location > - modem MD300 does not support this interface (it is not listed when calling org.freedesktop.DBus.Introspectable.Introspect) > > org.freedesktop.ModemManager.Modem.Cdma > - modem MD300 does not support this interface > > org.freedesktop.ModemManager.Modem.Gsm > - fully implemented and tested > > org.freedesktop.ModemManager.Modem.Gsm.Card > - to be implemented > > org.freedesktop.ModemManager.Modem.Gsm.Contacts > - modem MD300 does not support this interface > > org.freedesktop.ModemManager.Modem.Gsm.Network > - fully implemented > - to be tested: methods Register and SetApn > - still not working (gives error): method scan > - modem MD300 does not support: method GetBand and probably method SetBand too > > > > org.freedesktop.ModemManager.Modem.Gsm.SMS > > > - modem MD300 does not support method List and probably all other methods in this interface. > > > > org.freedesktop.ModemManager.Modem.Gsm.Hso > > > - modem MD300 does not support this interface > > > > > > org.freedesktop.ModemManager.Modem.Gsm.Ussd > > > - modem MD300 does not support this interface > > > Diffs > ----- > > /trunk/KDE/kdebase/workspace/libs/solid/control/CMakeLists.txt 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/backends/fakenet/fakeaccesspoint.h 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/backends/fakenet/fakeaccesspoint.cpp 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/backends/fakenet/fakenetworkinterface.h 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/backends/fakenet/fakenetworkinterface.cpp 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/ifaces/CMakeLists.txt 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/ifaces/modemmanager.h PRE- CREATION > /trunk/KDE/kdebase/workspace/libs/solid/control/ifaces/modemmanager.cpp PRE-CREATION > /trunk/KDE/kdebase/workspace/libs/solid/control/ifaces/networkgsminterface.h 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/ifaces/networkinterface.h 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/modemmanager.h PRE- CREATION > /trunk/KDE/kdebase/workspace/libs/solid/control/modemmanager.cpp PRE- CREATION > /trunk/KDE/kdebase/workspace/libs/solid/control/modemmanager_p.h PRE- CREATION > /trunk/KDE/kdebase/workspace/libs/solid/control/networkgsminterface.h 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/networkgsminterface.cpp 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/networkinterface.h 1128694 > /trunk/KDE/kdebase/workspace/libs/solid/control/networkinterface.cpp 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/CMakeLists.txt 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/accesspoint.h 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/CMakeLists.txt 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/hand-edits.diff 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/introspection/all.xml 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/introspection/mm- gsm-network.xml PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/introspection/mm- manager-client.xml PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/mm-gsm- networkinterface.h PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/mm-gsm- networkinterface.cpp PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/mm-manager- clientinterface.h PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/mm-manager- clientinterface.cpp PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/modemmanager- types.h PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/dbus/modemmanager- types.cpp PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/modemmanager.h PRE- CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/modemmanager.cpp PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/modemmanager_p.h PRE-CREATION > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkgsminterface.h 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkgsminterface.cpp 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkgsminterface_p.h 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkinterface.h 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkinterface.cpp 1128694 > /trunk/KDE/kdebase/workspace/solid/networkmanager-0.7/networkinterface_p.h 1128694 > /trunk/KDE/kdebase/workspace/solid/wicd/networkinterface.h 1128694 > > Diff: http://reviewboard.kde.org/r/3769/diff > > > Testing > ------- > > > Thanks, > > Lamarque > > _______________________________________________ Kde-hardware-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-hardware-devel
