Avoid compilation warnings about deprecated call #359

milouse · 2023-07-24T20:41:29Z

As of NetworkManager 1.22, a lot of function should be asynchronously called.

This MR remove all the compilation warning still visible (at least on Archlinux). Maybe it’s only preparatory stuff for Debian/Ubuntu distros.

danirabbit

Hey thanks for your branch! I left a few inline comments

src/Views/VPNPage.vala

danirabbit · 2023-08-01T16:46:01Z

src/Widgets/DeviceItem.vala

@@ -137,6 +137,9 @@ namespace Network.Widgets {
                        subtitle = _("Enabled (auto mode)");
                        status_image.icon_name = "user-available";
                        break;
+                    default:


I think we want to make sure we're actually handling all the possible cases here rather than just ignoring a warning

I’m not sure what it is all about.

The Utils.CustomMode seems to contain unrelated things: the PROXY_* constants and 2 HOTSPOT_* constants. In the case of this switch_status function, it looks like being called only from 3 places:

Widgets/DeviceItem.vala:49 & 51

Views/ProxyPage.vala:97

Views/VPNPage.vala:170

None of these call are dealing with HOTSPOT_* related stuff.

That’s why I proposed to silently skip possibilities which should never occur. However a more robust/clean solution would be to remove the HOTSPOT_* constants from the CustomMode enum if we are not using them anymore? It looks like those constants have been forgotten in #176

And if we remove them from CustomMode, maybe we should rename this enum to ProxyMode?

What do you think?

Ah okay yeah, that makes sense since hotspot is handled already we should remove those from CustomMode and I agree makes sense to rename this to ProxyMode

danirabbit · 2023-08-01T16:49:44Z

src/Widgets/WifiMenuItem.vala

+                /* Could be a lot of various status (for now, one of:
+                 * `DISCONNECTED', `IP_CHECK', `CONFIG', `UNKNOWN',
+                 * `UNAVAILABLE', `IP_CONFIG', `SECONDARIES', `UNMANAGED',
+                 * `NEED_AUTH', `DEACTIVATING'). Showing the spinner might let
+                 * end-user try to have a look to what is going on. */
+                spinner.active = true;


This seems like we should handle this more carefully as well instead of just silencing the warning. It seems like a spinner isn't appropriate for states like "disconnected" or "unavailable" for example

I’ll see what I can propose to better wrap all those cases.

milouse · 2023-08-07T06:54:18Z

Thank you very much for you review, I’ll check that!

danirabbit

Hey sorry it took me so long to follow up on this, I didn't see your replies for some reason 😓

danirabbit · 2023-08-29T19:28:50Z

src/Widgets/DeviceItem.vala

@@ -137,6 +137,9 @@ namespace Network.Widgets {
                        subtitle = _("Enabled (auto mode)");
                        status_image.icon_name = "user-available";
                        break;
+                    default:


Ah okay yeah, that makes sense since hotspot is handled already we should remove those from CustomMode and I agree makes sense to rename this to ProxyMode

milouse · 2023-08-30T12:04:56Z

Hey sorry it took me so long to follow up on this, I didn't see your replies for some reason 😓

No problem. I’ll change the code in this direction then.

We still have one last issue regarding various wifi state. I didn’t take time either to look for a better solution than just activating the spinner. If I don’t find more time or a working solution for now, I might just rollback my change and just insert a TODO for a better skilled person (I’m very poor when it comes to design/UX).

milouse · 2023-08-30T12:54:57Z

Ok, I just take time to do it now. I also work on missing NM.DeviceState. From my point of view it was finally not that hard. However I did introduce new translation terms, don’t know if you are ok with that or not.

milouse · 2023-08-30T13:01:39Z

src/Widgets/WifiMenuItem.vala

@@ -158,20 +158,41 @@ public class Network.WifiMenuItem : Gtk.ListBoxRow {

        hide_item (error_img);
        spinner.active = false;
-
-        connect_button_revealer.reveal_child = true;
+        connect_button_revealer.reveal_child = false;


Connect button should be hidden by default (if we know there is a problem or the device is not usable, we should not let the user infinitely click on something broken).

milouse · 2023-08-30T13:02:23Z

src/Widgets/WifiMenuItem.vala

+            case NM.DeviceState.UNAVAILABLE: // the device is managed by NetworkManager, but is not available for use
+            case NM.DeviceState.UNMANAGED: // the device is recognized, but not managed by NetworkManager
+            case NM.DeviceState.FAILED: // the device failed to connect to the requested network and is cleaning up the connection request
+            case NM.DeviceState.SECONDARIES: // the device is waiting for a secondary connection (like a VPN) which must activated before the device can be activated


All those states reflect a locked/failed state

milouse · 2023-08-30T13:03:45Z

src/Widgets/WifiMenuItem.vala

                show_item (error_img);
                state_string = _("Could not be connected to");
                break;
-            case NM.DeviceState.PREPARE:
+            case NM.DeviceState.NEED_AUTH: // the device requires more information to continue connecting to the requested network


This one is also a failed state, but the resolution is quite different: we leave the connect button as a possibility for the end-user to be shown again a log in popup. I also introduce a new translation term to explain what is wrong.

milouse · 2023-08-30T13:04:17Z

src/Widgets/WifiMenuItem.vala

+            case NM.DeviceState.CONFIG: // the device is connecting to the requested network
+            case NM.DeviceState.IP_CONFIG: // the device is requesting IPv4 and/or IPv6 addresses and routing information from the network
+            case NM.DeviceState.IP_CHECK: // the device is checking whether further action is required for the requested network connection
+            case NM.DeviceState.PREPARE: // the device is preparing the connection to the network


All those states reflect an ongoing connection, thus I put them together.

milouse · 2023-08-30T13:08:57Z

src/Widgets/WifiMenuItem.vala

+                connect_button_revealer.reveal_child = true;
+                break;
+            case NM.DeviceState.ACTIVATED: // the device has a network connection, either local or global
+                // Nothing to do


Activated is now the default state. Nothing to do as hidding the connect button is now the default behavior.

I introduced new steps as:

deactivating: the same as the "prepare" step, but in reverse, when the device is disconnecting. Thus it also trigger the spinner. I also added the new translation term "Disconnecting" as a reverse for "Connecting".

disconnected: this is a default "nothing to do" state, when the device is just disconnected, without any issue ongoing. As we hide the connect button by default, this case is used to show it. Finally I plug the "unknown" state to it instead of the first "error" state as unknown can be for a lot of other reason, not necessarily an error. Let see in the futur if it would be better plugged to an error state.

milouse · 2023-08-30T13:10:54Z

src/Widgets/DeviceItem.vala

-               }
+                    case Utils.ProxyMode.INVALID:
+                        subtitle = _("Disabled (error)");
+                        status_image.icon_name = "user-busy";


I use the "user-busy" icon to display the error, as for the failed DeviceState on line 117 above in order to be coherent.

milouse · 2023-11-02T13:28:47Z

Rebased on last master.

danirabbit · 2024-01-11T20:28:53Z

Hey sorry for not reviewing promptly. This has been hard to review because it changes several unrelated things that all have to be reviewed separately. It's much better to propose separate branches with unrelated changes.

danirabbit

Can you revert the changes in WifiMenuItem and propose that separately? That doesn't seem related to resolving the deprecation warnings with NM

As of NM 1.22

milouse · 2024-02-23T10:22:11Z

Hi, and verry sorry for the long silence.

I just rebased the branch on the last main state, updating it for gtk4 port.

Can you revert the changes in WifiMenuItem and propose that separately? That doesn't seem related to resolving the deprecation warnings with NM

Actually my changes there were to address a warning about missing states in the switch/case statement, thus for me it’s still related to the topic of this MR (removing as much warning as possible). However I can understand you prefer to separate each fix step. Can you confirm your prefer 3 different MR?

danirabbit · 2024-03-12T17:27:09Z

@milouse Yes please 3 separate pull requests would make a lot more sense here. The change to use async methods I can easily approve and merge immediately but the changes to the proxy enum I think might need some further investigation and the changes to the wifitem definitely needs a completely separate review as well

danirabbit requested changes Aug 1, 2023

View reviewed changes

milouse force-pushed the remove-warnings branch from 9e2078a to c4dd7ff Compare August 7, 2023 06:58

danirabbit reviewed Aug 29, 2023

View reviewed changes

milouse force-pushed the remove-warnings branch from 2c4b0b3 to 88caa01 Compare August 30, 2023 12:58

milouse commented Aug 30, 2023

View reviewed changes

milouse force-pushed the remove-warnings branch from 88caa01 to c44a288 Compare October 10, 2023 20:51

milouse force-pushed the remove-warnings branch from c44a288 to fca5cc1 Compare November 2, 2023 13:26

danirabbit requested changes Jan 11, 2024

View reviewed changes

milouse added 3 commits February 23, 2024 10:26

Avoid compilation warnings about deprecated call

f73c579

As of NM 1.22

Remove old HOTSPOT constant and rename CustomMode to ProxyMode

e0cbedd

Support all current states of NM.DeviceState

a9a6aec

milouse force-pushed the remove-warnings branch from 33ca7f9 to a9a6aec Compare February 23, 2024 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid compilation warnings about deprecated call #359

Avoid compilation warnings about deprecated call #359

milouse commented Jul 24, 2023

danirabbit left a comment

danirabbit Aug 1, 2023

milouse Aug 7, 2023

danirabbit Aug 29, 2023

danirabbit Aug 1, 2023

milouse Aug 7, 2023

milouse commented Aug 7, 2023

danirabbit left a comment

danirabbit Aug 29, 2023

milouse commented Aug 30, 2023

milouse commented Aug 30, 2023

milouse Aug 30, 2023

milouse Aug 30, 2023

milouse Aug 30, 2023

milouse Aug 30, 2023

milouse Aug 30, 2023

milouse Aug 30, 2023

milouse commented Nov 2, 2023

danirabbit commented Jan 11, 2024

danirabbit left a comment

milouse commented Feb 23, 2024

danirabbit commented Mar 12, 2024

Avoid compilation warnings about deprecated call #359

Are you sure you want to change the base?

Avoid compilation warnings about deprecated call #359

Conversation

milouse commented Jul 24, 2023

danirabbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milouse commented Aug 7, 2023

danirabbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milouse commented Aug 30, 2023

milouse commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milouse commented Nov 2, 2023

danirabbit commented Jan 11, 2024

danirabbit left a comment

Choose a reason for hiding this comment

milouse commented Feb 23, 2024

danirabbit commented Mar 12, 2024