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

Wifi-menu: Multi-interface support and man page. #178

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

Conversation

tetrakist
Copy link

This adds support for multiple interfaces to wifi-menu. It detects multiple wireless interfaces and presents the user with a menu, allowing them to choose which one to use. It also adds a man page for wifi-menu

Copy link
Owner

@joukewitteveen joukewitteveen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to improve netctl! The proposed changes are welcome, yet they need some tweaks before they can go in. Let's first look at the wifi-menu code.

docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
src/wifi-menu Show resolved Hide resolved
Copy link
Owner

@joukewitteveen joukewitteveen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your quick response and apologies for being quite a bit slower ;-).

There's a lot of comments this time, but don't get me wrong: I am very happy with your contribution. Criticizing is just so much easier than writing.

docs/wifi-menu.8.txt Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
docs/wifi-menu.8.txt Show resolved Hide resolved
docs/wifi-menu.8.txt Outdated Show resolved Hide resolved
src/wifi-menu Outdated Show resolved Hide resolved
@tetrakist tetrakist force-pushed the multi-interface-support branch from c46add5 to 7964888 Compare May 26, 2020 18:59
Copy link
Owner

@joukewitteveen joukewitteveen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor nitpicks left. If you agree with them, please accept them and squash this PR to one or two (first with the changes to wifi-menu, second with the man-page) commits. After that we're done!

Thanks a lot!

*wifi-menu* allows a user to interactively connect to a wireless network
over INTERFACE using an existing netctl profile, or an automatically
generated profile for basic configurations. If only one wireless
interface is available, INTERFACE can be omitted. If no interface is
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interface is available, INTERFACE can be omitted. If no interface is
interface is available, INTERFACE can be omitted. If no interface is

(for consistency)

Comment on lines +19 to +20
over INTERFACE using an existing netctl profile, or an automatically
generated profile for basic configurations. If only one wireless
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that only basic configurations are supported for generating profiles is already stressed in the next paragraph. No need to state it twice.

Suggested change
over INTERFACE using an existing netctl profile, or an automatically
generated profile for basic configurations. If only one wireless
on INTERFACE using a netctl profile. In case no netctl profile is available, one can be generated.
If only one wireless

(pending reflow to the proper number of columns)


*wifi-menu* is only able to generate netctl profiles for simple network
configurations. Profiles for more complex configurations must be set up
manually, or through the use of other tools.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
manually, or through the use of other tools.
manually.

This man-page is about wifi-menu, so we assume the intent is to use netctl.

-----
The program may display a black screen for up to a minute when starting a
connection.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how best to word a statement about control by netctl-auto and switching profiles, so I will leave that for you to do as you see fit.

Maybe here? I'd appreciate your comments. Rather than me just adding a commit on top of yours, I think it is nice to make this part of your commit.

Suggested change
Starting a connection on an interface that is controlled by *netctl-auto* will
cause *wifi-menu* to switch to the selected network. The interface will continue
to be controlled by *netctl-auto*.

Comment on lines +36 to +37
Do not echo password, and store derived pre-shared key
in profile instead of plaintext password.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with "do not echo password". The "Show asterisks..." bit is just cut-and-paste from the output of --help. ;)

Ah, thanks for this remark! That reminds me of why I chose this wording. The 'do not echo' wording applies more to what you see for instance when you type your password on a login terminal (i.e. nothing at all).
Let's revert to something that closely resembles your original idea.

Suggested change
Do not echo password, and store derived pre-shared key
in profile instead of plaintext password.
Show asterisks for the characters of the password and store the password
as a hexadecimal pre-shared key.

Comment on lines +71 to +72
The program may display a black screen for up to a minute when starting a
connection.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this one got lost somewhere. If you disagree with my preference to make 'minute' refer less to a specific unit of time, I'd like to hear why.

Suggested change
The program may display a black screen for up to a minute when starting a
connection.
Establishing a connection may take a minute. While establishing a connection,
nothing is shown on screen.

FILES
-----
+/etc/netctl+::
Directory where created netctl profiles are stored.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one got lost too. Here, 'generated' is more specific than 'created' and 'netctl' is implied.

Suggested change
Directory where created netctl profiles are stored.
Directory where generated profiles are stored.

Comment on lines +18 to +19
-o, --obscure Do not echo password, and store derived pre-shared
key in profile instead of plaintext password.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about this ;-).

Suggested change
-o, --obscure Do not echo password, and store derived pre-shared
key in profile instead of plaintext password.
-o, --obscure Show asterisks for the characters of the password
and store the password as a hexadecimal string

@joukewitteveen
Copy link
Owner

Please let me know if you accept the final proposed changes. If you want me to, I can squash the commits for you.

@joukewitteveen
Copy link
Owner

I took the least intrusive part of this MR: 1e08e03.
If you want the interface selection dialog bits (which I am not against), please rebase on (or restart from) master. That way, we can get attribution right. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants