Proposal for changing the API to a more object oriented style

[majorz]

I am opening a proposal for changing the current API to a more object oriented style. It achieves encapsulation of internal properties and methods of all structs. For example the current NetworkManager struct has public methods related to the D-Bus internals, which users of the library should not have access to.

Additionally I propose not passing a manager reference to each call - we can use a private shared reference counted pointer to the D-Bus connection holding struct in all the structs representing our public API (NetworkManager, Connection, Device, etc.).

I changed the names of some methods to use the verbs from the D-Bus NetworkManager spec. With some methods such as connect_to_access_point and create_hotspot I did not use the original AddAndActivateConnection, since this one is a multipurpose method that goes beyond what our methods do.

Here is pseudocode of the proposed API change:

NetworkManager
    stop_service(timeout: u64)                  -> Result<ServiceState, Error>
    start_service(timeout: u64)                 -> Result<ServiceState, Error>
    get_service_state()                         -> Result<ServiceState, Error>

    set_method_timeout(&self, timeout: u64)     -> ()

    get_state(&self)                            -> Result<NetworkManagerState, Error>
    get_connectivity(&self)                     -> Result<Connectivity, Error>
    is_networking_enabled(&self)                -> Result<bool, Error>
    is_wireless_enabled(&self)                  -> Result<bool, Error>

    get_devices(&self)                          -> Result<Vec<Device>, Error>
    get_device_by_interface(&self,
            interface: &str)                    -> Result<Device, Error>
    get_connections(&self)                      -> Result<Vec<Connection>, Error>
    get_active_connections(&self)               -> Result<Vec<Connection>, Error>

Connection
    settings(&self)                             -> &ConnectionSettings
    get_state(&self)                            -> Result<ConnectionState, Error>

    activate(&self)                             -> Result<ConnectionState, Error>
    deactivate(&self)                           -> Result<ConnectionState, Error>
    delete(&self)                               -> Result<(), Error>

Device
    device_type(&self)                          -> &DeviceType
    interface(&self)                            -> &String
    get_state(&self)                            -> Result<DeviceState, Error>

    as_wifi_device(&self)                       -> Option<WifiDevice>>
    connect(&self)                              -> Result<DeviceState, Error>
    disconnect(&self)                           -> Result<DeviceState, Error>

WifiDevice
    get_access_points(&self)                    -> Result<Vec<AccessPoints>, Error>
    connect_to_access_point(&self,
            access_point: &AccessPoint,
            password: Option<&str>)             -> Result<(Connection, ConnectionState), Error>
    create_hotspot(&self,
            ssid: &str,
            password: Option<&str>)             -> Result<(Connection, ConnectionState), Error>

Currently enabling and disabling a connection looks like this:

use network_manager::{manager, connection};

let manager = manager::new();

let connections = connection::list(&manager).unwrap();
let connection = connections[0];

connection::disable(&manager, connection, 10).unwrap();
connection::enable(&manager, connection, 10).unwrap();

println!("{:?}", connection.state);

With the new API it will look like this:

use network_manager::NetworkManager;

let manager = NetworkManager::new();

let connections = manager.get_connections().unwrap();
let connection = connections[0];

connection.activate().unwrap();
let state = connection.deactivate().unwrap();

println!("{:?}", state);

[jbaldwinroberts]

@majorz I have a few questions:

• How is connect_to_access_point different from connection::activate?
• I think a hotspot should be created the same way as a normal connection - after all the only difference is the fields used in the configuration.
• Can you give an example of how I would create and then activate a connection please.
• Can you show the function inputs as well as the outputs please.
• Which of the returned types would implement clone, if any?
• Can you show an example of scanning for access points please.
• Can you give a rough time estimate?
• How would I change a connection setting after it was created? Instead of using getters and setters could we have the settings as properties on the object instead?

[majorz]

How is connect_to_access_point different from connection::activate?

The wifi_device.connect_to_access_point(&AccessPoint, &str, i32) will be the equivalent of connection::create(&NetworkManager, &Device, &AccessPoint, &str, i32)). connection::activate will stay the same.

If the name is confusing how about WifiDevice::create_access_point_connection or WifiDevice::create_connection_to_access_point? Maybe we can rename the other method to WifiDevice::create_hotspot_connection?

I think a hotspot should be created the same way as a normal connection - after all the only difference is the fields used in the configuration.

Does the answer to the previous question provide an answer? Please, let me know more if not?

Can you show the function inputs as well as the outputs please.

I will edit the post above when I finish this comment.

Which of the returned types would implement clone, if any?

With the strategy from paragraph 2 we will be able to implement clone on all of them, since they will share the same D-Bus connection.

Can you show an example of scanning for access points please.

I will post a separate comment on this one, since I forgot to bring up a scanning issue I encountered when I was going through the network manager API and I did not include the method for that reason, but only a get_access_points version.

Can you give a rough time estimate?

A couple of days, since this change will mostly be renaming existing functions to methods and moving code around.

How would I change a connection setting after it was created? Instead of using getters and setters could we have the settings as properties on the object instead?

This is a very good question, I will check what options do we have, since Rust does not have property wrappers like Python does.

[majorz]

WifiDevice::scan method

The network manager has three related methods and properties:

• RequestScan() - Request the device to scan
• GetAllAccessPoints() - Get the list of all access points visible to this device, including hidden ones for which the SSID is not yet known.
  • AccessPoints - List of object paths of access point visible to this wireless device.

Currently we only use the AccessPoints property in wifi::scan, which is not very correct, so I just renamed this one to WifiDevice::get_access_points. We can add RequestScan() as a WifiDevice::request_scan, but we do not know when newly scanned devices will be available unless we add support for the AccessPointAdded and AccessPointAdded signals.

[jbaldwinroberts]

Can you give an example of how I would create and then activate a connection please. I really think we should just have AddConnection, ActivateConnection, AddAndActivateConnection, DeleteConnection which work for all connection types (included hotspot)

[majorz]

getters/setters

I found an interesting section about that in the conventions RFC: https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis

My understanding is that we need to rename Connection::get_settings(&self) -> &ConnectionSettings to Connection::settings(&self) -> &ConnectionSettings. And do the same for Device::get_device_type and Device::get_interface.

I updated the three methods above to reflect that. But haven't done that for methods that invoke the network manager to return results.

[majorz]

Can you give an example of how I would create and then activate a connection please.

It will look like this:

let manager = NetworkManager::new();
let device = manager.get_device_by_interface("wlo1").as_wifi_device().unwrap();
let access_points = device.get_access_points().unwrap();
let access_point = find_the_right_access_point(&access_points);
let (connection, state) = device.connect_to_access_point(access_point, Some("passw0rd")).unwrap();

With the current API we do the following (assuming we have a device::get_device_by_interface function):

let manager = manager::new();
let device = device::get_device_by_interface("wlo1").unwrap();
let access_points = wifi::scan(&manager, &device).unwrap();
let access_point = find_the_right_access_point(&access_points);
let connection = connection::create(&manager, &device, access_point, "passw0rd", 10).unwrap();

I really think we should just have AddConnection, ActivateConnection, AddAndActivateConnection, DeleteConnection which work for all connection types (included hotspot)

AddAndActivateConnection is split into two more convenient methods in the WifiDevice implementation - I did it like that when I saw how nmcli provides "device wifi connect" and "device wifi hotspot" commands. I think we can add at a later point AddConnection and AddAndActivateConnection as NetworkManager::add_connection and NetworkManager::add_and_activate_connection methods. Those will accept more advanced options - nmcli also has "connection add" that does that.

[jbaldwinroberts]

OK LTGM @majorz

Last thing is that Device::deactivate is missing or the naming is not consistent e.g.

    activate(&self)                                -> Result<DeviceState, Error>
    disconnect(&self)                           -> Result<DeviceState, Error>

[majorz]

Last thing is that Device::deactivate is missing or the naming is not consistent e.g.

I doubled checked with nmcli and they are using connect and disconnect as names. connect uses NetworkManager::ActivateConnection internally, disconnect uses Device::disconnect.

I will update the API above to have connect/disconnect symmetry.