Created attachment 304435 [details] [review]
initial patch

Review of attachment 304435 [details] [review]:

::: gio/gnetworkmonitornm.c
@@ +143,3 @@
+        return FALSE;
+      default:
+        g_warning ("Unknown NM metered state %d", nm_metered);

Probably this "default" case never hits.

And if it hits, then it hits because NM has a bug at this place (unlikely) or it added a new enum value (more-likely).

In the latter case I think you don't want to warn because otherwise updating NM could result in such warnings. How about just making a best guess and silently "return TRUE"?

I think the idea for the warning would be so that someone reports a bug and we add the new enum rather than guessing. I just copied the precedent for g_network_monitor_get_network_available() but I'm happy to change either default.

The two usual behaviours for enumerations are:

 * g_assert_not_reached(); this way, if NM is updated to add a new enumeration value and GLib isn't updated to handle it, we'll get a SIGABRT.
 * g_warn_if_reached() + return default value; this way, if NM is updated to add a new enumeration value and GLib isn't updated to handle it, we'll get a critical warning, but the library will go on.

Both assertion and warning can be disabled at compile time, but we usually enforce crashing to avoid undefined state after a critical warning (even if developers keep trudging on and never fix critical warnings).

My personal preference is to use g_assert_not_reached(), but it's up to the GIO maintainers to choose.

Comment on attachment 304435 [details] [review]
initial patch

>+ * g_network_monitor_get_network_metered:
>+ * @monitor: the #GNetworkMonitor
>+ *
>+ * Checks if the network is metered. "Metered" here means that the
>+ * traffic flowing through the connection is subject to limitations,
>+ * for example set by service providers.

"Limitations" is too vague. You should give an example scenario of why you'd want to call this function.

>+   * If this information is not available then no networks willl be

typo "willl"

>+   * See also #GNetworkMonitor::network-available.

Only one colon. (properties have one colon, signals have two)

>+      case 0: /* unknown */
>+      case 1: /* yes */
>+      case 3: /* guess-yes */
>+        return TRUE;

Returning TRUE when NM says "unknown" seems like a bad idea, since in general machines running NM/GNOME are more likely to be on unmetered networks than metered ones. (And also, we default to FALSE in the unknown-because-you're-not-running-NM case, so we should return FALSE in the unknown-when-you-are-running-NM case too.)

[Actually, given that there are "guess-yes" and "guess-no" values, I don't think NM should ever return "unknown"; "unknown" requires the application to guess what it should do, but NM knows more about the network configuration and so can always make a better guess than the application can.]

>+      /* this is only available post 1.0 */

"post NM 1.0" (just to be completely clear)

>+  if (new_network_metered != nm->priv->network_metered)
>+    {
>+      nm->priv->network_metered = new_network_metered;
>+      g_object_notify (G_OBJECT (nm), "network-available");

cut+paste-o s/available/metered/

>+  g_object_class_override_property (gobject_class, PROP_NETWORK_METERED, "network-metered");

You need to override this in GNetworkMonitorBase too, or else gobject will spew warnings when you call g_network_monitor_get_network_metered() on a netlink or dummy GNetworkMonitor.

oh, i have no opinion on the default: case question

Review of attachment 304435 [details] [review]:

::: gio/gnetworkmonitor.c
@@ +368,3 @@
+   * system network management API should be used instead.
+   *
+   * If this information is not available then no networks willl be

typo: willl

@@ +371,3 @@
+   * marked as metered.
+   *
+   * See also #GNetworkMonitor::network-available.

Only one : for properties

::: gio/gnetworkmonitornm.c
@@ +203,3 @@
+    {
+      nm->priv->network_metered = new_network_metered;
+      g_object_notify (G_OBJECT (nm), "network-available");

This should notify network-metered, I guess

Created attachment 307840 [details] [review]
gio: Add network metered information to GNetworkMonitor

Add a property to GNetworkMonitor indicating if the network
is metered, e.g. subject to limitations set by service providers, such
as billing per unit of traffic sent, or limiting the amount of traffic
per month).

The default value is FALSE.

Here’s an updated patch which addresses the review comments so far, though the changes I’ve made to the GNetworkMonitor:network-metered documentation could do with some scrutiny.

One question, Richard: why did you go with a boolean for GNetworkMonitor:network-metered rather than exposing the full multi-state value from NetworkMonitor? I suspect that other backends could map whatever multi-state they use onto it, and it would give the application a bit more information to work with when choosing whether to devour bandwidth.

I’m tempted to expose the multi-state.

Created attachment 307842 [details] [review]
gio: Add network metered information to GNetworkMonitor

Add a property to GNetworkMonitor indicating if the network
is metered, e.g. subject to limitations set by service providers, such
as billing per unit of traffic sent, or limiting the amount of traffic
per month).

The default value is G_NETWORK_METERED_UNKNOWN; other permissible values
follow the NetworkManager API, which seems general enough to be useful
to other monitor backends.

---

Here’s another version of the patch which exposes the multi-state from NetworkMonitor in the most obvious way possible. I think NM’s multi-state values should be generally useful for other monitor backends, and feel that exposing this extra few bits of information to apps could be useful. For example, you might want to query the user before starting a big file download if the metering status is guess-no (“it’s possible that this will cost you a bomb”) but not if the status is no.

Review of attachment 307840 [details] [review]:

This looks ok

Review of attachment 307842 [details] [review]:

Lets keep it simple and in line with network-available. If you want to get all the information, talk to NetworkManager directly.

Attachment 307840 [details] pushed as a80e7db - gio: Add network metered information to GNetworkMonitor

(In reply to Matthias Clasen from comment #13)
> Attachment 307840 [details] pushed as a80e7db - gio: Add network metered
> information to GNetworkMonitor

Not quite sure what happened here but it looks like attachment #304435 [details] (Richard’s initial patch) got committed instead of attachment #307840 [details].

I have applied the inter-diff as commit 6b652b1a2e7c6f67e9576e4331da76971d54cc68.