Skip to content
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

Fix "Network.app" issues #150

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

CocoCR300
Copy link

@CocoCR300 CocoCR300 commented Feb 6, 2023

  • Change icon based on "signal level".
  • Don't ask for the password if the network is already saved to etc/wpa_supplicanf.conf.
  • Check why the wireless interface is being disabled on app startup. (ifconfig down and up)
  • Display a check icon on the currently connected network, a lock icon on the ones protected with password and the signal level.
  • Upper limit and ASCII check for Wi-Fi passwords (Network block - psk. Seems like non-ASCII characters are accepted)
  • Other issues...

@CocoCR300 CocoCR300 marked this pull request as draft February 6, 2023 20:40
@CocoCR300
Copy link
Author

CocoCR300 commented Feb 6, 2023

I used a different set of icons because the ones being used were very hard to see on the menu, but the new ones are color blue... Maybe that's an issue, and also the way of determining the signal level was taken from wifimgr and I'm not totally sure if it works well here.

* We need to prevent another dialog asking for password being displayed, or check authentication failures in other way
@CocoCR300
Copy link
Author

CocoCR300 commented Feb 7, 2023

For some reason the fix made on ifconfig down and up is messing with my current configuration, after bringing the interface down and up it can't connect to any network, and scanning for networks gives nothing. Also, adding up to the line ifconfig_wlan0 in /etc/rc.conf produces the same behavior.

@CocoCR300 CocoCR300 marked this pull request as ready for review February 10, 2023 18:32
@CocoCR300
Copy link
Author

CocoCR300 commented Feb 10, 2023

@probonopd, I think this is ready for review, there are still some things to fix and I want to do, but I want your opinion on the current work, specially the part about watching /var/log/messages (to check authentication failures), if that is something you want to avoid.
About the icons, we need a new set, the current one only includes some for the different signal levels, nothing for "acquiring connection" or "no signal", and mixing different icons looks a bit bad.
There's also a new password dialog!

* Identify currently connected network by BSSID
* Remove added network if user clicks "Cancel" on password request dialog
@probonopd
Copy link
Member

@CocoCR300 in a quick test, this has been working very well for me. I really like how you placed the signal strength and encryption icons to the right-hand side. Very welcome improvements. I hope the QFileSystemWatcher will prove robust enough. I guess the only way to find this out is to try it on a larger scale, so I am inclined to merge as soon as you think it's ready for merging.

As for the color icons, there is a way in Qt to render them as grayscale. I am using the same for the icons in the global menu bar at the right-hand corner (left to the time):

helloSystem/Menu@c13548d

I don't have the PyQt5 equivalent handy, but I am sure you will figure it out (if not, feel free to ping me anytime).

Great stuff! 👍

@probonopd
Copy link
Member

Do you think you could add in wired, too? Keep in mind that wired network devices (e.g., USB) can come and go, too.

@CocoCR300
Copy link
Author

I can try with USB tethering from my cellphone, that's the only thing I have available since my laptop doesn't have an ethernet connector, I hope that's enough to implement something.

@probonopd
Copy link
Member

helloSystem 0.8.1 should have USB Tethering work out of the box, at least for Android phones.

@CocoCR300
Copy link
Author

@probonopd I added a pretty simple support for wired interfaces, I didn't found a way to get a "friendly name" for the interface and the current code just checks for "ue" in the interface name to say if it's "USB Ethernet", everything else is named as "Ethernet (interface_name)".
Also, there's still an issue with the menu item selected background color that I couldn't fix.

@probonopd
Copy link
Member

probonopd commented Feb 19, 2023

Great work.

It seems that we should check the return code of wpa_cli scan_results a bit more carefully, since on a system without a wireless network card, it currently crashes when you click on the Wired Ethernet icon with:

"QSystemTrayIcon::setVisible: No Icon set\nTraceback (most recent call last):\n File \"/tmp/Utilities/./Under Construction/Network.app/Network\", line 199, in show_menu\n self.updateMenu()\n File \"/tmp/Utilities/./Under Construction/Network.app/Network\", line 377, in updateMenu\n if \"wpa_state=COMPLETED\" in self.status_lines:\nAttributeError: 'NetworkMenu' object has no attribute 'status_lines'\n"

Can you please try on a system that has no wireless NIC or has it disabled?

@probonopd
Copy link
Member

probonopd commented Feb 20, 2023

I have 2 USB Ethernet adapters in my system, but only one is plugged into a cable, and hence only one is active - which one?

image

% dmesg | grep Ethernet
ugen1.6: <Realtek USB-C Dock Ethernet> at usbus1
ure0: <Realtek USB-C Dock Ethernet, class 0/0, rev 2.10/31.03, addr 5> on usbus1
ue0: <USB Ethernet> on ure0
ue0: Ethernet address: ...
ue1: <USB Ethernet> on axe0
ue1: Ethernet address: ...

% ifconfig
ue0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500        options=68009b<RXCSUM,TXCSUM,VLAN_MTU,VLAN_HWTAGGING,VLAN_HWCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
        ether ...
        inet6 ...%ue0 prefixlen 64 scopeid 0x2
        media: Ethernet autoselect (none)
        status: no carrier
        nd6 options=23<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL>
ue1: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        options=8000b<RXCSUM,TXCSUM,VLAN_MTU,LINKSTATE>
        ether ...
        inet6 ...%ue1 prefixlen 64 scopeid 0x3
        inet6 ... prefixlen 64 autoconf
        inet 192.168.0... netmask 0xffffff00 broadcast 192.168.0.255
        media: Ethernet autoselect (100baseTX <full-duplex>)
        status: active
        nd6 options=23<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL>

@probonopd
Copy link
Member

Great work!

Instead of launching lots of ifconfig processes which is quite some overhead, maybe we could use something like https://www.freshports.org/net/py-netifaces (sudo pkg install net/py-netifaces)? Looking at the description, it looks easy to use: https://pypi.org/project/netifaces/.

Also we don't need to do the polling all the time; only while the menu is open. Like we are doing in Volume.app,

self.tray.activated.connect(self.onClicked) # Refresh each time the menu is clicked (left click)
# Also refresh the list of devices when the context menu is requested (right click)
self.menu.aboutToShow.connect(self.refreshMenu)

@CocoCR300
Copy link
Author

CocoCR300 commented Feb 21, 2023

Thanks!
Great, I thought you didn't wanted to add dependencies, using a library will be a big relief for this.
I'll make those changes, but something is bugging me out, I don't really know how to handle more than one network device being active, it didn't worked past days, but now I'm connected to Wi-Fi and using USB tethering to connect to internet (Not really useful for me, but it can be for some people). And yes, seems like there can be only one default route in the routing table, so one of the interfaces should have like an "internet" icon (which doesn't exist in the current icon set internet-web-browser-symbolic can be the one).

@probonopd
Copy link
Member

probonopd commented Feb 21, 2023

In general you are right, I am trying to avoid dependencies. But all those launched processes for the polling... are not really helping for great performance; so I think it's an OK tradeoff here to introduce this dependency if you agree.

…i network, don't disable wireless interface while switching to wired
@CocoCR300
Copy link
Author

I couldn't do much use of the library you suggested, it doesn't give all the details needed for the application (interface status, interfaces being up/down). It would also be beneficial to use a library to interact with wpa_supplicant.
About switching between network interfaces, only the root user can alter the routing table :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants