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

[WIP] Fix network overview for NetworkManager users #1317

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

clanig
Copy link

@clanig clanig commented Jan 3, 2023

Please allow me to propose a solution to make navigating the network section more flawlessly.

Problem

The user is displayed a message when entering the tab Overview in the Network Settings of YaST and by acknowledging that message, forced to switch to tab Global Options. Apparently this is done to prevent a user from accessing controls that can't be used. This solution might be acceptable in most cases, although it is not possible to check the list of network interfaces anymore, even without the intention to change anything.

However, in case of the ncurses UI it breaks the option to use a combination of TAB, Arrow Keys, Enter to navigate instead of Alt+<KEY> shortcuts. (See bug report.)

https://bugzilla.suse.com/show_bug.cgi?id=1205715

Solution

This PR would disable the buttons Add, Edit, Delete on the tab Overview as well as the interfaces table if NetworkManager is used for configuration.
It will not force the user to Global Options anymore when acknowledging the warning. Instead it will inform about the network setup method option in Global Options. As a result, the user is still able to see the properties of the network interfaces on the Overview page.
The popup message is not shown anymore. Instead, in the box which usually displays the interface properties, an information is displayed that Network Manager is managing the interfaces.
In the help of the Overview page, an information about fixing this by selecting Wicked is given.

Reason for hiding all interfaces:

It looks that the interfaces displayed might differ from reality if NM is used. C.f. https://bugzilla.suse.com/show_bug.cgi?id=1205715#c7
From my perspective it makes sense to handle this separately if required. As a result, these are hidden as well with the current code.

Testing

The code was tested manually on openSUSE Tumbleweed.

Request for comments

  1. Please give some comments on the proposed change.
  2. This would be my first contribution to the project - if I missed a side effect, please let me know.
  3. I changed the warning message to only tell that NetworkManager is used. If :network_manager as backend means that the network could also be disabled, please let me know as well. To me it appears that we know that the network is not disabled but just NetworkManager is used if that is the value.

Screenshots

Screenshot from 2023-01-12 09-52-29

Screenshot from 2023-01-12 09-53-15

Screenshot from 2023-01-12 09-55-25

Screenshot from 2023-01-12 09-55-39

@shundhammer
Copy link
Contributor

See also https://bugzilla.suse.com/show_bug.cgi?id=1205715#c8

AFAICS that PR changes the behavior of that module: It just disables the "Overview" tab. But it still posts that pop-up (only once when the module starts up?).

I am not sure if that still fulfills the requirements that we surely got back when that pop-up was introduced. A user might just click it away because it always appears when you start the module, and might then wonder why he can't switch to the "Overview" tab.

That whole behavior with a pop-up that ALWAYS appears when you start the module has always been a user interface atrocity: Something always popping up during normal operation, when there was no error, is just wrong.

But then additionally hiding the problem by just disabling a whole tab without telling the user what's going on makes the problem just worse.

IMHO if we really want to spend time on this (as if we don't have anything more important to do than to take care of another fringe case like cursor keys in NCurses when selecting a tab - who cares!):

Instead of just disabling the whole tab, in case Network Manager is active, there should be a bold-faced message IN the tab content: "Controlled by Network Manager" as the first thing in the tab content, and the rest of the tab content (the lists of devices etc.) should still be there, but disabled.

(Note: a VBox or HBox can also get an ID, and you can disable part of a dialog by disabling that container widget).

E.g.

[Global Options][Overview][Hostname/DNS][Routing]

Managed by Network Manager

+----------------------------------------------------
| Name   | IP Address  | Device  | Note
|
| br0      DHCP          br0
| RTL811   NONE          etho0     included in br0
|
.
.
|
+----------------------------------------------------

+----------------------------------------------------
|
|
|
.
.
+-----------------------------------------------------

And, like in the PR, NOT automatically switch to another tab (which is a user interface atrocity all by itself) so the user can actually read the message. And NOT post a pop-up at all, of course: The user will realize that parts cannot be configured when switching to that tab, and there will not be that horrible pop-up to get in the way.

That pop-up has always been wrong.

@ancorgs
Copy link
Contributor

ancorgs commented Jan 9, 2023

The only thing I miss in your "screenshot" is the part that indicates you can switch to Wicked to get those controls enabled by using the "Global Options" tab.

@shundhammer
Copy link
Contributor

The only thing I miss in your "screenshot" is the part that indicates you can switch to Wicked to get those controls enabled by using the "Global Options" tab.

That's intentional: It would be something to go to the help text. Static texts in dialogs should be minimalistic, not a replacement for the help text.

Also it should be self-explanatory how to do that: It's in the first tab, after all.

@clanig clanig force-pushed the fix_network_overview_for_nm branch from 9241c51 to eddf3a4 Compare January 12, 2023 08:42
@clanig
Copy link
Author

clanig commented Jan 12, 2023

Thanks for your comments. I have updated the code and the description according to your suggestions.

Due to possible inconsistencies, currently it makes the most sense to me not displaying any interfaces.

@shundhammer
Copy link
Contributor

The table still makes sense, even if the network interfaces are managed by Network Manager (which is the default for most home users).

@teclator
Copy link
Contributor

The table still makes sense, even if the network interfaces are managed by NetworkManager (which is the default for most home users).

Well, we have to discuss about it because we are showing basically the ifcfg files in /etc/sysconfig/network/ which could be a different configuration than the NetworkManager one...

@mchf
Copy link
Member

mchf commented Jan 12, 2023

From my POV after first quick reading:

  • i dislike(d) that switching to another tab too, but
  • in some cases it might be useful to have some content here, so
  • i don't see empty overview as a proper solution as I can imagine some bug reports about empty overview coming right after merging it. I'd prefer either (slightly easier solution) removing all irrelevant tabs (running NM? -> we cannot help you with Overview tab -> we will not make it available for you). We already use this concept of "on demand tabs" sometimes in y2-network (e.g. for s390) so we can extend it. Or even better is to fill everything with proper data - do not read config from wicked / netconfig when using NM but read it from NM directly and display it r/o to the user.

@clanig
Copy link
Author

clanig commented Jan 12, 2023

I agree that the best option would be reading the correct values from Network Manager. I will take a look and check the feasibility.

@clanig clanig marked this pull request as draft March 6, 2023 14:44
@clanig clanig changed the title RFC: Fix network overview for NetworkManager users [WIP] Fix network overview for NetworkManager users Mar 6, 2023
@clanig
Copy link
Author

clanig commented Mar 6, 2023

I have not forgotten this PR. I still intend to work on it soon but I am currently a bit busy.

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.

5 participants