-
Notifications
You must be signed in to change notification settings - Fork 42
Feature(panel): add Wi-Fi network menu with connection management #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hi and thanks for your PR! However I am wondering why we need to invoke nmcli, are there really no dbus interfaces for connecting to wifi? |
|
I couldn't connect to any network using the dbus API. It shows the available networks but crashes while trying to connect to any SSID. here's the code block: void WayfireNetworkInfo::attempt_connect_ssid(const std::string &ssid,
const std::string &password)
{
if (ssid.empty() || ssid.find_first_not_of(" \t\r\n") == std::string::npos) {
pop_status_label.set_text("Invalid SSID");
return;
}
try {
// Initialization (omitted for brevity, assume success)
auto bus = Gio::DBus::Connection::get_sync(Gio::DBus::BusType::SYSTEM);
auto nm = Gio::DBus::Proxy::create_sync(bus,
"org.freedesktop.NetworkManager",
"/org/freedesktop/NetworkManager",
"org.freedesktop.NetworkManager");
// --- Find Wi-Fi device (Mocked for compilation, assume real logic is used) ---
// NOTE: In production code, you must replace this mock with the actual device detection.
std::string wifi_device_path = "/org/freedesktop/NetworkManager/Devices/1";
// Safety check for production device path detection
if (wifi_device_path.empty()) {
pop_status_label.set_text("No Wi-Fi device");
return;
}
// --- Build settings with corrected dictionary structures (a{sv}) ---
// 1. Create SSID byte array (type 'ay'). This is a full reference.
std::vector<guchar> ssid_bytes(ssid.begin(), ssid.end());
GVariant *ssid_array = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE,
ssid_bytes.data(), ssid_bytes.size(), 1);
// 2. Build 'connection' dictionary (a{sv})
GVariantBuilder conn_builder;
g_variant_builder_init(&conn_builder, G_VARIANT_TYPE("a{sv}"));
g_variant_builder_add(&conn_builder, "{sv}", "type", g_variant_new_string("802-11-wireless"));
g_variant_builder_add(&conn_builder, "{sv}", "id", g_variant_new_string(ssid.c_str()));
gchar *uuid = g_uuid_string_random();
g_variant_builder_add(&conn_builder, "{sv}", "uuid", g_variant_new_string(uuid));
g_free(uuid);
GVariant *connection_dict = g_variant_builder_end(&conn_builder); // Full reference
// 3. Build '802-11-wireless' dictionary (a{sv})
GVariantBuilder wifi_builder;
g_variant_builder_init(&wifi_builder, G_VARIANT_TYPE("a{sv}"));
// g_variant_new_variant consumes ssid_array (transfer-full).
g_variant_builder_add(&wifi_builder, "{sv}", "ssid", g_variant_new_variant(ssid_array));
g_variant_builder_add(&wifi_builder, "{sv}", "mode", g_variant_new_string("infrastructure"));
GVariant *wifi_dict = g_variant_builder_end(&wifi_builder); // Full reference
// 4. Build '802-11-wireless-security' dictionary (a{sv})
GVariant *sec_dict = nullptr;
if (!password.empty()) {
GVariantBuilder sec_builder;
g_variant_builder_init(&sec_builder, G_VARIANT_TYPE("a{sv}"));
g_variant_builder_add(&sec_builder, "{sv}", "key-mgmt", g_variant_new_string("wpa-psk"));
g_variant_builder_add(&sec_builder, "{sv}", "psk", g_variant_new_string(password.c_str()));
sec_dict = g_variant_builder_end(&sec_builder); // Full reference
}
// 5. Build top-level array (a{sa{sv}}) - The entries for this array are of type {sa{sv}}
// Create the entries using g_variant_new(), which creates a FLOATING reference
// and consumes the full references of the dictionary variants (connection_dict, etc.).
GVariant *conn_entry = g_variant_new("{sa{sv}}", "connection", connection_dict);
GVariant *wifi_entry = g_variant_new("{sa{sv}}", "802-11-wireless", wifi_dict);
// Now add the floating entries to the top builder using g_variant_builder_add_value,
// which consumes the floating reference. This is the safest way.
GVariantBuilder top_builder;
g_variant_builder_init(&top_builder, G_VARIANT_TYPE("a{sa{sv}}"));
g_variant_builder_add_value(&top_builder, conn_entry);
g_variant_builder_add_value(&top_builder, wifi_entry);
if (sec_dict) {
GVariant *sec_entry = g_variant_new("{sa{sv}}", "802-11-wireless-security", sec_dict);
g_variant_builder_add_value(&top_builder, sec_entry);
}
GVariant *settings = g_variant_builder_end(&top_builder);
if (!settings) {
pop_status_label.set_text("Failed to build NetworkManager settings variant.");
return;
}
// Glib::VariantContainerBase takes ownership (transfer-full)
Glib::VariantContainerBase settings_wrap(settings, true);
// --- Call AddAndActivateConnection (D-Bus signature: (a{sa{sv}}, o, o)) ---
// Create the GVariant tuple: settings, device_path, connection_path
// Use g_variant_ref() to increment the reference count of the existing settings variant
// so the new tuple construction can consume one reference.
GVariant *args_final = g_variant_new("(@a{sa{sv}}oo)",
g_variant_ref(settings_wrap.gobj()),
wifi_device_path.c_str(),
"/");
// Glib::VariantContainerBase takes ownership of the tuple (args_final).
Glib::VariantContainerBase args_wrap(args_final, true);
// --- Call AddAndActivateConnection ---
nm->call_sync("AddAndActivateConnection", args_wrap);
pop_status_label.set_text("Connecting...");
update_active_connection();
}
catch (const Glib::Error &e) {
std::cerr << "Connect failed: " << e.what() << std::endl;
pop_status_label.set_text("Failed: " + Glib::ustring(e.what()));
}
catch (...) {
pop_status_label.set_text("Unknown error");
}
}And before crash it outputs these errors on console: (wf-panel:7112): Gtk-CRITICAL **: 01:18:19.327: gtk_box_append: assertion 'gtk_widget_get_parent (child) == NULL' failed
(wf-panel:7112): Gtk-WARNING **: 01:18:19.342: Theme parser error: <data>:1:28-33: Expected a valid color.
Font fallback .battery {font: 1rem default;}
Font fallback .clock {font: 1rem default;}
(wf-panel:7112): GLib-CRITICAL **: 01:18:27.500: g_variant_builder_end: assertion 'valid_builder' failed
(wf-panel:7112): GLib-CRITICAL **: 01:18:27.500: g_variant_get_type: assertion 'value != NULL' failed
(wf-panel:7112): GLib-CRITICAL **: 01:18:27.500: g_variant_type_is_array: assertion 'g_variant_type_check (type)' failed
(wf-panel:7112): GLib-CRITICAL **: 01:18:27.500: g_variant_get_type_string: assertion 'value != NULL' failed
(wf-panel:7112): GLib-ERROR **: 01:18:27.500: g_variant_new: expected array GVariantBuilder but the built value has type '(null)'
[1] 7112 trace trap (core dumped) wf-panel
|
|
Hi! Thank's for your PR! Also please use |
|
I'm stuck here. I'm new to Dbus Interface and Networkmanager api. The panel now stable but can't connect to any network. Connect failed: GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Type of message, “(a{sv}oo)”, does not match expected type “(a{sa{sv}}oo)”// --- Each section (a{sv}) ---
std::map<Glib::ustring, Glib::VariantBase> connection_map;
connection_map["type"] = Glib::Variant<Glib::ustring>::create("802-11-wireless");
connection_map["id"] = Glib::Variant<Glib::ustring>::create(ssid);
connection_map["uuid"] = Glib::Variant<Glib::ustring>::create(uuid);
auto connection_dict = Glib::Variant<std::map<Glib::ustring, Glib::VariantBase>>::create(connection_map);
std::map<Glib::ustring, Glib::VariantBase> wifi_map;
wifi_map["ssid"] = ssid_variant;
wifi_map["mode"] = Glib::Variant<Glib::ustring>::create("infrastructure");
auto wifi_dict = Glib::Variant<std::map<Glib::ustring, Glib::VariantBase>>::create(wifi_map);
Glib::Variant<std::map<Glib::ustring, Glib::VariantBase>> sec_dict;
if (!password.empty()) {
std::map<Glib::ustring, Glib::VariantBase> sec_map;
sec_map["key-mgmt"] = Glib::Variant<Glib::ustring>::create("wpa-psk");
sec_map["psk"] = Glib::Variant<Glib::ustring>::create(password);
sec_dict = Glib::Variant<std::map<Glib::ustring, Glib::VariantBase>>::create(sec_map);
}
// --- Wrap into a{sa{sv}} ---
std::map<Glib::ustring, Glib::Variant<std::map<Glib::ustring, Glib::VariantBase>>> settings_map;
settings_map["connection"] = connection_dict;
settings_map["802-11-wireless"] = wifi_dict;
if (!password.empty())
settings_map["802-11-wireless-security"] = sec_dict;
auto settings = Glib::Variant<std::map<
Glib::ustring,
Glib::Variant<std::map<Glib::ustring, Glib::VariantBase>>
>>::create(settings_map);
// --- Object paths (must be 'o' type) ---
auto device_path = Glib::Variant<Glib::DBusObjectPathString>::create(wifi_device_path);
auto ap_path = Glib::Variant<Glib::DBusObjectPathString>::create("/");
// --- Final tuple (a{sa{sv}} o o) ---
std::vector<Glib::VariantBase> tuple_items = { settings, device_path, ap_path };
auto args = Glib::VariantContainerBase::create_tuple(tuple_items);
// --- Call ---
nm->call_sync("AddAndActivateConnection", args);
|
|
@rummanz I am also not very familiar with glib but after some experimentation I think I found what the issue is. The code should look like this (I deleted a few lines but you will get the idea I hope): // --- Each section (a{sv}) ---
std::map<Glib::ustring, Glib::VariantBase> connection_map;
connection_map["type"] = Glib::Variant<Glib::ustring>::create("802-11-wireless");
// --- Wrap into a{sa{sv}} ---
std::map<Glib::ustring, std::map<Glib::ustring, Glib::VariantBase>> settings_map;
settings_map["connection"] = connection_map;
auto settings = Glib::Variant<std::map<
Glib::ustring,
std::map<Glib::ustring, Glib::VariantBase>>>::create(settings_map);Pay attention to the types: you don't have to build a variant out of every nested dictionary, because that erases the types. You could even make the code like this: using ConnectionSettings = std::map<Glib::ustring, std::map<Glib::ustring, Glib::VariantBase>>; // this is actually a{sa{sv}}
ConnectionSettings settings;
settings["connection"]["type"] = Glib::Variant<Glib::ustring>::create("802-3-ethernet"); // the Variant::create() means to pack the whole value for sending over the wire, no need to do that for subdictionaries
auto settings_v = Glib::Variant<ConnectionSettings>::create(settings); |
|
@ammen99 Thanks for your suggestion. Now it's working as expected. I need to enhance the UI a little bit then I will push the changes. |
ammen99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few things I noticed on a first glance :)
… improvements. -Trigger an async wifi scan in backgroud while the popover menu opens. -Return to main menu after connect to a network. -Connected network information now shows on the top of scan result with a disconnect button. -Added a detail button to show connected network information e.g. security, strength, frequency, ip address, subnet mask, dns, gateway. -Right click to open gnome-control-center and left click to show popover menu.
|
@rummanz Formatting seems quite odd at times, most likely due to the high level of nested callbacks. Maybe we should split out some of the functionality into separate functions? I think that will improve readability as well. Also btw I don't think you need the connection to network manager on dbus to be async itself, but only the wifi scan, as that is what takes a long time. The connection itself can be sync (like we already have), and you need to connect once at startup, so it is fine for that operation to be sync. This will also simplify the code a bit :) |
-trigger_wifi_scan_async: Uses the existing `nm_proxy` and does everything synchronously (devices → Wi‑Fi device → RequestScan), then schedules a short timeout before invoking the callback. Only the scan wait is async now. -get_available_networks_async: Enumerates devices/APs synchronously via `call_sync` and cached properties, then calls the provided callback once with the full list. No nested async proxy creation/calls anymore.
|
@ammen99 Thanks for your valuable feedback. I have refactored the code and removed async call to network manager dbus api. Only network scan is now async. |
Thanks! I will test it tomorrow on my laptop where I have a wi-fi card and will do a final review. |
ammen99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this very briefly on my laptop, some things I noticed:
-
The widget, in 'idle' state (i.e not showing menu or anything) does not seem to display the output of the active wifi connection, as the current implementation in master does. I am not sure why this part was removed from the PR.
-
When using left-click, it triggers a scan and then the networks are populated. I think we can cache the old networks (from a previous scan) and then update the list once we get the new results. Maybe depending on how old the previous scan was. I mostly mean it for cases where you click, then dismiss, then immediately open the menu again, it looks weird that we have to wait for the results to open up. At the very least, I think while we are scanning, we should display the currently connected network.
-
I seem to be getting my Wi-Fi network twice, once as the current connection, once as an option to connect to it, that is probably not what we want?
-
The popover seems to be bigger than it needs to be to accomodate the list of networks, I am not sure whether there is anything we can do about it.
| return vstr.get(); | ||
| } | ||
|
|
||
| std::string get_strength_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we have the same thing as above, I feel like all of this should be one function returning whichever properties we want, not multiple functions with the same if cascade.
| int strength = 0; // Wi-Fi signal strength (0–100) | ||
| bool secured = false; // True if the network requires authentication | ||
| }; | ||
| // Map 0-100 strength to GNOME symbolic icon names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me like we have a different gradient (here 5-30-55-80, below for the colors 0-25-40-100), maybe we should put both in the same list (i.e change status_color so that it includes the icon name as well, then have one function to compute both?)
I think the real issue here is theming, in the gtk4 port (cc @trigg) the color gradient was removed, but it seems we forgot to remove the code for it. Now you have fixed it, but I suppose that the css properties won't work anymore (seems like you removed the classes in this PR?):
wf-shell/src/panel/widgets/network.cpp
Lines 274 to 289 in 57a68cb
| void WayfireNetworkInfo::update_status() | |
| { | |
| std::string description = info->get_connection_name(); | |
| status.set_text(description); | |
| button.set_tooltip_text(description); | |
| status.get_style_context()->remove_class("excellent"); | |
| status.get_style_context()->remove_class("good"); | |
| status.get_style_context()->remove_class("weak"); | |
| status.get_style_context()->remove_class("none"); | |
| if (status_color_opt) | |
| { | |
| status.get_style_context()->add_class(info->get_strength_str()); | |
| } | |
| } |
| return "network-wireless-signal-ok-symbolic"; | ||
| if (value > 5) | ||
| return "network-wireless-signal-weak-symbolic"; | ||
| return "network-wireless-signal-none-symbolic"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is also duplicated in get_icon_name() in the wifi connection type implementation.
Implemented a Wi-Fi menu to the panel that allows users to view, select, and connect to wireless networks using nmcli. Includes password prompt, live status updates via DBus support.