Network: Sort network menu items w/o deleting

* Delete dropped out networks.
* Add in newly discovered networks.
* Add static (aka class) compare method to WirelessNetworkMenuItem
  that is used to sort items by signal strength descending.

Add == operator to wireless_network struct to determine if
existing items have a known network attached.

Remove the non-network items from the menu, save them, sort
network menu items, then add non-network items back into the
menu.

Update NetworkStatus preflet to use same compare method as Network
preflet. signal_strength_compare function had a bool return value
instead of int which worked to sort items the first time, but does
not work on successive compares.

By not deleting and recreating the menu items each Pulse(),
the Network preflet no longer crashes on update. The menu flashes
on update still but doesn't crash.

Fixes #12024

Change-Id: Ie5b22cea4e66350b9c5df8e3b8de266ede50ad6d
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4243
Reviewed-by: John Scipione <[email protected]>
Reviewed-by: waddlesplash <[email protected]>
Reviewed-by: Axel Dörfler <[email protected]>

headers/os/net/NetworkDevice.h

@@ -7,6 +7,7 @@
 
 
 #include <net/if.h>
+#include <string.h>
 
 #include <NetworkAddress.h>
 
@@ -24,6 +25,18 @@ struct wireless_network {
 	uint32				cipher;
 	uint32				group_cipher;
 	uint32				key_mode;
+
+	bool operator==(const wireless_network& other) {
+		return strncmp(name, other.name, 32) == 0
+			// ignore address difference
+			&& noise_level == other.noise_level
+			&& signal_strength == other.signal_strength
+			&& flags == other.flags
+			&& authentication_mode == other.authentication_mode
+			&& cipher == other.cipher
+			&& group_cipher == other.group_cipher
+			&& key_mode == other.key_mode;
+	}
 };
 
 // flags

src/apps/networkstatus/NetworkStatusView.cpp

@@ -37,7 +37,6 @@
 #include <Locale.h>
 #include <MenuItem.h>
 #include <MessageRunner.h>
-#include <NetworkDevice.h>
 #include <NetworkInterface.h>
 #include <NetworkRoster.h>
 #include <PopUpMenu.h>
@@ -75,20 +74,7 @@ const uint32 kMinIconWidth = 16;
 const uint32 kMinIconHeight = 16;
 
 
-//	#pragma mark -
+//	#pragma mark - NetworkStatusView
-
-
-static bool
-signal_strength_compare(const wireless_network &a,
-	const wireless_network &b)
-{
-	if (a.signal_strength == b.signal_strength)
-		return strcmp(a.name, b.name) > 0;
-	return a.signal_strength > b.signal_strength;
-}
-
-
-//	#pragma mark -
 
 
 NetworkStatusView::NetworkStatusView(BRect frame, int32 resizingMode,
@@ -393,80 +379,61 @@ NetworkStatusView::MouseDown(BPoint point)
 	menu->SetAsyncAutoDestruct(true);
 	menu->SetFont(be_plain_font);
 	BString wifiInterface;
-	BNetworkDevice wifiDevice;
+	BNetworkDevice device;
 
-	// Add interfaces
+	if (!fInterfaceStatuses.empty()) {
+		for (std::map<BString, int32>::const_iterator it
+				= fInterfaceStatuses.begin(); it != fInterfaceStatuses.end();
+				++it) {
+			const BString& name = it->first;
 
-	for (std::map<BString, int32>::const_iterator it
+			// we only show network of the first wireless device we find
-		= fInterfaceStatuses.begin(); it != fInterfaceStatuses.end(); ++it) {
+			if (wifiInterface.IsEmpty()) {
-		const BString& name = it->first;
+				device.SetTo(name);
-
+				if (device.IsWireless())
-		BString label = name;
+					wifiInterface = name;
-		label += ": ";
+			}
-		label += kStatusDescriptions[
-			_DetermineInterfaceStatus(name.String())];
-
-		BMessage* info = new BMessage(kMsgShowConfiguration);
-		info->AddString("interface", name.String());
-		menu->AddItem(new BMenuItem(label.String(), info));
-
-		// We only show the networks of the first wireless device we find.
-		if (wifiInterface.IsEmpty()) {
-			wifiDevice.SetTo(name);
-			if (wifiDevice.IsWireless())
-				wifiInterface = name;
 		}
 	}
 
-	if (!fInterfaceStatuses.empty())
+	// Add wireless networks, if any, first so that we can sort the menu
-		menu->AddSeparatorItem();
-
-	// Add wireless networks, if any
 
 	if (!wifiInterface.IsEmpty()) {
 		std::set<BNetworkAddress> associated;
 		BNetworkAddress address;
+		wireless_network network;
 		uint32 cookie = 0;
-		while (wifiDevice.GetNextAssociatedNetwork(cookie, address) == B_OK)
+		while (device.GetNextAssociatedNetwork(cookie, address) == B_OK)
 			associated.insert(address);
 
+		int32 wifiCount = 0;
 		cookie = 0;
-		wireless_network network;
+		while (device.GetNextNetwork(cookie, network) == B_OK) {
-		typedef std::vector<wireless_network> WirelessNetworkVector;
-		WirelessNetworkVector wirelessNetworks;
-		while (wifiDevice.GetNextNetwork(cookie, network) == B_OK)
-			wirelessNetworks.push_back(network);
-
-		std::sort(wirelessNetworks.begin(), wirelessNetworks.end(),
-			signal_strength_compare);
-
-		int32 count = 0;
-		for (WirelessNetworkVector::iterator it = wirelessNetworks.begin();
-				it != wirelessNetworks.end(); it++) {
-			wireless_network &network = *it;
-
 			BMessage* message = new BMessage(kMsgJoinNetwork);
 			message->AddString("device", wifiInterface);
 			message->AddString("name", network.name);
 			message->AddFlat("address", &network.address);
 
-			BMenuItem* item = new WirelessNetworkMenuItem(network.name,
+			BMenuItem* item = new WirelessNetworkMenuItem(network, message);
-				network.signal_strength, network.authentication_mode, message);
 			menu->AddItem(item);
+			wifiCount++;
 			if (associated.find(network.address) != associated.end())
 				item->SetMarked(true);
-
-			count++;
 		}
-		if (count == 0) {
+
+		if (wifiCount == 0) {
 			BMenuItem* item = new BMenuItem(
 				B_TRANSLATE("<no wireless networks found>"), NULL);
 			item->SetEnabled(false);
 			menu->AddItem(item);
-		}
+		} else
+			menu->SortItems(WirelessNetworkMenuItem::CompareSignalStrength);
+
 		menu->AddSeparatorItem();
 	}
 
+	// add action menu items
+
 	menu->AddItem(new BMenuItem(B_TRANSLATE(
 		"Open network preferences" B_UTF8_ELLIPSIS),
 		new BMessage(kMsgOpenNetworkPreferences)));
@@ -475,6 +442,32 @@ NetworkStatusView::MouseDown(BPoint point)
 		menu->AddItem(new BMenuItem(B_TRANSLATE("Quit"),
 			new BMessage(B_QUIT_REQUESTED)));
 	}
+
+	// Add wired interfaces to top of menu
+	if (!fInterfaceStatuses.empty()) {
+		int32 wiredCount = 0;
+		for (std::map<BString, int32>::const_iterator it
+				= fInterfaceStatuses.begin(); it != fInterfaceStatuses.end();
+				++it) {
+			const BString& name = it->first;
+
+			BString label = name;
+			label += ": ";
+			label += kStatusDescriptions[
+				_DetermineInterfaceStatus(name.String())];
+
+			BMessage* info = new BMessage(kMsgShowConfiguration);
+			info->AddString("interface", name.String());
+			menu->AddItem(new BMenuItem(label.String(), info), wiredCount);
+			wiredCount++;
+		}
+
+		// add separator item between wired and wireless networks
+		// (or between wired networks and actions if no wireless found)
+		if (wiredCount > 0)
+			menu->AddItem(new BSeparatorItem(), wiredCount);
+	}
+
 	menu->SetTargetForItems(this);
 
 	ConvertToScreen(&point);

src/apps/networkstatus/WirelessNetworkMenuItem.cpp

@@ -6,27 +6,29 @@
 
 #include "WirelessNetworkMenuItem.h"
 
+#include <string.h>
+
 #include <Catalog.h>
-#include <NetworkDevice.h>
 #include <String.h>
 
 #include "RadioView.h"
 
+
 #undef B_TRANSLATION_CONTEXT
 #define B_TRANSLATION_CONTEXT "WirelessNetworkMenuItem"
 
 
-WirelessNetworkMenuItem::WirelessNetworkMenuItem(const char* name,
+WirelessNetworkMenuItem::WirelessNetworkMenuItem(wireless_network network,
-	int32 signalQuality, int32 authenticationMode, BMessage* message)
+	BMessage* message)
 	:
-	BMenuItem(name, message),
+	BMenuItem(network.name, message),
-	fQuality(signalQuality)
+	fNetwork(network)
 {
 	// Append authentication mode to label
 	BString label = B_TRANSLATE("%name% (%authenticationMode%)");
-	label.Replace("%name%", name, 1);
+	label.Replace("%name%", network.name, 1);
 	label.Replace("%authenticationMode%",
-		AuthenticationName(authenticationMode), 1);
+		AuthenticationName(network.authentication_mode), 1);
 
 	SetLabel(label.String());
 }
@@ -37,13 +39,6 @@ WirelessNetworkMenuItem::~WirelessNetworkMenuItem()
 }
 
 
-void
-WirelessNetworkMenuItem::SetSignalQuality(int32 quality)
-{
-	fQuality = quality;
-}
-
-
 BString
 WirelessNetworkMenuItem::AuthenticationName(int32 mode)
 {
@@ -99,5 +94,23 @@ WirelessNetworkMenuItem::DrawRadioIcon()
 	bounds.right -= 4;
 	bounds.bottom -= 2;
 
-	RadioView::Draw(Menu(), bounds, fQuality, RadioView::DefaultMax());
+	RadioView::Draw(Menu(), bounds, fNetwork.signal_strength,
+		RadioView::DefaultMax());
+}
+
+
+/*static*/ int
+WirelessNetworkMenuItem::CompareSignalStrength(const BMenuItem* a,
+	const BMenuItem* b)
+{
+	WirelessNetworkMenuItem* aItem = *(WirelessNetworkMenuItem**)a;
+	WirelessNetworkMenuItem* bItem = *(WirelessNetworkMenuItem**)b;
+
+	wireless_network aNetwork = aItem->Network();
+	wireless_network bNetwork = bItem->Network();
+
+	if (aNetwork.signal_strength == bNetwork.signal_strength)
+		return strncasecmp(aNetwork.name, bNetwork.name, 32);
+
+	return bNetwork.signal_strength - aNetwork.signal_strength;
 }

src/apps/networkstatus/WirelessNetworkMenuItem.h

@@ -7,20 +7,23 @@
 
 
 #include <MenuItem.h>
+#include <NetworkDevice.h>
 
 
 class WirelessNetworkMenuItem : public BMenuItem {
 public:
-								WirelessNetworkMenuItem(const char* name,
+								WirelessNetworkMenuItem(
-									int32 signalQuality, int32 authenticationMode,
+									wireless_network network,
 									BMessage* message);
 	virtual						~WirelessNetworkMenuItem();
 
-			void				SetSignalQuality(int32 quality);
+			wireless_network	Network() const { return fNetwork; }
-			int32				SignalQuality() const
+
-									{ return fQuality; }
 			BString 			AuthenticationName(int32 mode);
 
+	static	int					CompareSignalStrength(const BMenuItem* a,
+									const BMenuItem* b);
+
 protected:
 	virtual	void				DrawContent();
 	virtual	void				Highlight(bool isHighlighted);
@@ -28,7 +31,7 @@ protected:
 			void				DrawRadioIcon();
 
 private:
-			int32				fQuality;
+			wireless_network	fNetwork;
 };
 
 

src/preferences/network/InterfaceView.cpp

@@ -20,7 +20,6 @@
 #include <ControlLook.h>
 #include <LayoutBuilder.h>
 #include <NetworkAddress.h>
-#include <NetworkDevice.h>
 #include <StringForSize.h>
 #include <StringView.h>
 #include <TextControl.h>
@@ -243,7 +242,27 @@ InterfaceView::_Update(bool updateWirelessNetworks)
 	if (isWireless && updateWirelessNetworks) {
 		// Rebuild network menu
 		BMenu* menu = fNetworkMenuField->Menu();
-		menu->RemoveItems(0, menu->CountItems(), true);
+		int32 count = menu->CountItems();
+
+		// remove non-network items from menu and save them for later
+		BMenuItem* chooseItem = NULL;
+		BSeparatorItem* separatorItem = NULL;
+		if (count > 0 && strcmp(menu->ItemAt(0)->Label(),
+				B_TRANSLATE("Choose automatically")) == 0) {
+			// remove Choose automatically item
+			chooseItem = menu->RemoveItem((int32)0);
+			// remove separator item too
+			separatorItem = (BSeparatorItem*)menu->RemoveItem((int32)0);
+			count -= 2;
+		}
+
+		BMenuItem* noNetworksFoundItem = NULL;
+		if (menu->CountItems() > 0 && strcmp(menu->ItemAt(0)->Label(),
+				B_TRANSLATE("<no wireless networks found>")) == 0) {
+			// remove <no wireless networks found> item
+			noNetworksFoundItem = menu->RemoveItem((int32)0);
+			count--;
+		}
 
 		std::set<BNetworkAddress> associated;
 		BNetworkAddress address;
@@ -252,7 +271,6 @@ InterfaceView::_Update(bool updateWirelessNetworks)
 			associated.insert(address);
 
 		wireless_network network;
-		int32 count = 0;
 		cookie = 0;
 		if ((fPulseCount % 15) == 0
 				&& device.GetNextNetwork(cookie, network) != B_OK) {
@@ -270,36 +288,92 @@ InterfaceView::_Update(bool updateWirelessNetworks)
 			snooze(50 * 1000);
 		}
 
+		// go through menu items and remove networks that have dropped out
+		for (int32 index = 0; index < count; index++) {
+			WirelessNetworkMenuItem* networkItem =
+				dynamic_cast<WirelessNetworkMenuItem*>(
+					menu->ItemAt(index));
+			if (networkItem == NULL)
+				break;
+
+			bool networkFound = false;
+			cookie = 0;
+			while (device.GetNextNetwork(cookie, network) == B_OK) {
+				if (networkItem->Network() == network) {
+					networkFound = true;
+					break;
+				}
+			}
+
+			if (!networkFound) {
+				menu->RemoveItem(networkItem);
+				count--;
+			}
+		}
+
+		// go through networks and add new ones to menu
 		cookie = 0;
 		while (device.GetNextNetwork(cookie, network) == B_OK) {
-			BMessage* message = new BMessage(kMsgJoinNetwork);
+			bool networkFound = false;
+			for (int32 index = 0; index < count; index++) {
+				WirelessNetworkMenuItem* networkItem =
+					dynamic_cast<WirelessNetworkMenuItem*>(
+						menu->ItemAt(index));
+				if (networkItem == NULL)
+					break;
 
-			message->AddString("device", fInterface.Name());
+				if (networkItem->Network() == network) {
-			message->AddString("name", network.name);
+					// found it
-			message->AddFlat("address", &network.address);
+					networkFound = true;
+					if (associated.find(network.address) != associated.end())
+						networkItem->SetMarked(true);
+					break;
+				}
+			}
 
-			BMenuItem* item = new WirelessNetworkMenuItem(network.name,
+			if (!networkFound) {
-				network.signal_strength,
+				BMessage* message = new BMessage(kMsgJoinNetwork);
-				network.authentication_mode, message);
+				message->AddString("device", fInterface.Name());
-			if (associated.find(network.address) != associated.end())
+				message->AddString("name", network.name);
-				item->SetMarked(true);
+				message->AddFlat("address", &network.address);
-			menu->AddItem(item);
+				BMenuItem* item = new WirelessNetworkMenuItem(network,
+					message);
+				menu->AddItem(item);
+				if (associated.find(network.address) != associated.end())
+					item->SetMarked(true);
+			}
 
 			count++;
 		}
+
 		if (count == 0) {
-			BMenuItem* item = new BMenuItem(
+			// no networks found
-				B_TRANSLATE("<no wireless networks found>"), NULL);
+			if (noNetworksFoundItem != NULL)
-			item->SetEnabled(false);
+				menu->AddItem(noNetworksFoundItem);
-			menu->AddItem(item);
+			else {
+				BMenuItem* item = new BMenuItem(
+					B_TRANSLATE("<no wireless networks found>"), NULL);
+				item->SetEnabled(false);
+				menu->AddItem(item);
+			}
 		} else {
-			BMenuItem* item = new BMenuItem(
+			// sort items by signal strength
-				B_TRANSLATE("Choose automatically"), NULL);
+			menu->SortItems(WirelessNetworkMenuItem::CompareSignalStrength);
-			if (menu->FindMarked() == NULL)
+
-				item->SetMarked(true);
+			// add Choose automatically item to start
-			menu->AddItem(item, 0);
+			if (chooseItem != NULL) {
-			menu->AddItem(new BSeparatorItem(), 1);
+				menu->AddItem(chooseItem, 0);
+				menu->AddItem(separatorItem, 1);
+			} else {
+				BMenuItem* item = new BMenuItem(
+					B_TRANSLATE("Choose automatically"), NULL);
+				if (menu->FindMarked() == NULL)
+					item->SetMarked(true);
+				menu->AddItem(item, 0);
+				menu->AddItem(new BSeparatorItem(), 1);
+			}
 		}
+
 		menu->SetTargetForItems(this);
 	}