Bug 42288

+  service_name = g_strdup (field->raw_value_contents[0]);

If the form has a ServiceName field but no <value/> elements, will parsing the form fail, or will this crash?


+  if (field == NULL)
+    {
+      DEBUG ("ServiceName propery not present in data form; odd...");
+      goto out;
+    }
+
+  service_name = g_strdup (field->raw_value_contents[0]);
+
+out:

Eh, why not

   if (field != NULL)
     service_name = ...
   else
     DEBUG ("ServiceName...");

rather than goto? I don't think the goto makes it clearer.

Blah, if ServiceName is not going to be stashed in the same data form as the metadata, then it shouldn't be a data form. it should just be

  <service-name xmlns="im:telepathy:file-transfer:service" name='com.example.HiMum'/>

and… im: is not a URI scheme. http://telepathy.im/...

+      WockyStanza *tmp = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+          WOCKY_STANZA_SUB_TYPE_RESULT, NULL, NULL,
+          '(', "x",
+            ':', NS_X_DATA,
+            '@', "type", "result",
+            '(', "field",
+              '@', "var", "FORM_TYPE",
+              '@', "type", "hidden",
+              '(', "value",
+                '$', NS_TP_FT_METADATA_SERVICE,
+              ')',
+            ')',
+            '(', "field",
+              '@', "var", "ServiceName",
+              '(', "value",
+                '$', self->priv->service_name,
+              ')',
+            ')',
+          ')',
+          NULL);
+      WockyNode *x = wocky_node_get_first_child (wocky_stanza_get_top_node (tmp));
+      WockyNodeTree *tree = wocky_node_tree_new_from_node (x);
+
+      wocky_node_add_node_tree (file, tree);
+      g_object_unref (tree);
+      g_object_unref (tmp);

Ugh this is dumb. There should be a wocky_node_tree_build().


+          WockyNode *field = wocky_node_add_child (x, "field");
+
+          wocky_node_set_attribute (field, "var", (const gchar *) key);
+
+          wocky_node_add_child_with_content (field, "value",
+              (const gchar *) val);

wocky_node_add_build (x,
  '(', "field",
    '@', "var", key,
    '(', "value",
      '$', val,
    ')',
  ')', NULL);

or is that less clear?

I don't really see why the metadata-specific test is the one which explicitly sends no metadata. I guess it's testing edge cases…

+        requests_iface = dbus.Interface(self.conn, cs.CONN_IFACE_REQUESTS)

Just use self.conn.Requests

+add_file_transfer_channel_class (GPtrArray *arr,
+    gboolean metadata)

Giving the variable a more descriptive name like "include_metadata_properties" might be nice.

+      service_name_value = tp_g_value_slice_new (G_TYPE_STRING);
+      g_value_set_string (service_name_value, service_name_str);

is secret code for tp_g_value_slice_new_string (service_name_str);

The huge test is huge.


    if expect_disco:
        # Gabble looks up our capabilities
        event = q.expect('stream-iq', to=contact, query_ns=ns.DISCO_INFO)
        query_node = xpath.queryForNodes('/iq/query', event.stanza)[0]
        assert query_node.attributes['node'] == \
            client + '#' + c['ver']

        # send good reply
        result = make_result_iq(stream, event.stanza)
        query = result.firstChildElement()
        query['node'] = client + '#' + c['ver']

        for f in features:
            feature = query.addElement('feature')
            feature['var'] = f

        stream.send(result)

This is secret code for caps_helper.expect_disco(); caps_helper.send_disco_reply(). In fact, most of receive_caps in that test is just caps_helper.presence_and_disco().

All the dbus.Dictionary(a.items() + {...}.items()) stuff is really noisy. def dict_union(a, b) maybe?

The test looks fine, I guess, it's just hard to read.

(In reply to comment #1)
> +  service_name = g_strdup (field->raw_value_contents[0]);
> 
> If the form has a ServiceName field but no <value/> elements, will parsing the
> form fail, or will this crash?

Let's be sure. I added another patch.

> +  if (field == NULL)
> +    {
> +      DEBUG ("ServiceName propery not present in data form; odd...");
> +      goto out;
> +    }
> +
> +  service_name = g_strdup (field->raw_value_contents[0]);
> +
> +out:
> 
> Eh, why not
> 
>    if (field != NULL)
>      service_name = ...
>    else
>      DEBUG ("ServiceName...");
> 
> rather than goto? I don't think the goto makes it clearer.

OK, done.

> Blah, if ServiceName is not going to be stashed in the same data form as the
> metadata, then it shouldn't be a data form. it should just be
> 
>   <service-name xmlns="im:telepathy:file-transfer:service"
> name='com.example.HiMum'/>

We discussed this in real life and I thought you conceded that using another data form just made things easier?

> and… im: is not a URI scheme. http://telepathy.im/...

Okay. :-)

> +      WockyStanza *tmp = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> +          WOCKY_STANZA_SUB_TYPE_RESULT, NULL, NULL,
> +          '(', "x",
> +            ':', NS_X_DATA,
> +            '@', "type", "result",
> +            '(', "field",
> +              '@', "var", "FORM_TYPE",
> +              '@', "type", "hidden",
> +              '(', "value",
> +                '$', NS_TP_FT_METADATA_SERVICE,
> +              ')',
> +            ')',
> +            '(', "field",
> +              '@', "var", "ServiceName",
> +              '(', "value",
> +                '$', self->priv->service_name,
> +              ')',
> +            ')',
> +          ')',
> +          NULL);
> +      WockyNode *x = wocky_node_get_first_child (wocky_stanza_get_top_node
> (tmp));
> +      WockyNodeTree *tree = wocky_node_tree_new_from_node (x);
> +
> +      wocky_node_add_node_tree (file, tree);
> +      g_object_unref (tree);
> +      g_object_unref (tmp);
> 
> Ugh this is dumb. There should be a wocky_node_tree_build().

I agree. I opened bug #42433.

> +          WockyNode *field = wocky_node_add_child (x, "field");
> +
> +          wocky_node_set_attribute (field, "var", (const gchar *) key);
> +
> +          wocky_node_add_child_with_content (field, "value",
> +              (const gchar *) val);
> 
> wocky_node_add_build (x,
>   '(', "field",
>     '@', "var", key,
>     '(', "value",
>       '$', val,
>     ')',
>   ')', NULL);
> 
> or is that less clear?

Well let's do it and see.

> I don't really see why the metadata-specific test is the one which explicitly
> sends no metadata. I guess it's testing edge cases…

Yes. It tests that no dataform is sent. I didn't want it to send an empty dataform in that case.

> +        requests_iface = dbus.Interface(self.conn, cs.CONN_IFACE_REQUESTS)
> 
> Just use self.conn.Requests

OK!

> +add_file_transfer_channel_class (GPtrArray *arr,
> +    gboolean metadata)
> 
> Giving the variable a more descriptive name like "include_metadata_properties"
> might be nice.

OK!

> +      service_name_value = tp_g_value_slice_new (G_TYPE_STRING);
> +      g_value_set_string (service_name_value, service_name_str);
> 
> is secret code for tp_g_value_slice_new_string (service_name_str);

YEAAAHHH.

> The huge test is huge.

Why not?

>     if expect_disco:
>         # Gabble looks up our capabilities
>         event = q.expect('stream-iq', to=contact, query_ns=ns.DISCO_INFO)
>         query_node = xpath.queryForNodes('/iq/query', event.stanza)[0]
>         assert query_node.attributes['node'] == \
>             client + '#' + c['ver']
> 
>         # send good reply
>         result = make_result_iq(stream, event.stanza)
>         query = result.firstChildElement()
>         query['node'] = client + '#' + c['ver']
> 
>         for f in features:
>             feature = query.addElement('feature')
>             feature['var'] = f
> 
>         stream.send(result)
> 
> This is secret code for caps_helper.expect_disco();
> caps_helper.send_disco_reply(). In fact, most of receive_caps in that test is
> just caps_helper.presence_and_disco().

Yep, I replaced lots of it with just that.

> All the dbus.Dictionary(a.items() + {...}.items()) stuff is really noisy. def
> dict_union(a, b) maybe?

I guess, done.

Branch updated, thanks for the review!

> ft-client-caps test: change client nameft-metadata
> 
> I've no idea why but if caps/tube-caps.py is run before this test (it
> has the same idea and design as this test was copied from it) then
> this one fails due to the same client name?! Another day...

I think this is probably because the caps end up in the cache so Gabble doesn't make the disco query that the test expects.

(In reply to comment #2)
> (In reply to comment #1)
> > +  if (field == NULL)
> > +    {
> > +      DEBUG ("ServiceName propery not present in data form; odd...");
> > +      goto out;
> > +    }
> > +
> > +  service_name = g_strdup (field->raw_value_contents[0]);
> > +
> > +out:
> > 
> > Eh, why not
> > 
> >    if (field != NULL)
> >      service_name = ...
> >    else
> >      DEBUG ("ServiceName...");
> > 
> > rather than goto? I don't think the goto makes it clearer.
> 
> OK, done.

No it's not, but in the course of fixing the above complaint you made the goto justify its own existence. :)

> > Blah, if ServiceName is not going to be stashed in the same data form as the
> > metadata, then it shouldn't be a data form. it should just be
> > 
> >   <service-name xmlns="im:telepathy:file-transfer:service"
> > name='com.example.HiMum'/>
> 
> We discussed this in real life and I thought you conceded that using another
> data form just made things easier?

It means one less XEP to write I guess, but it offends me at some level. :)

> > and… im: is not a URI scheme. http://telepathy.im/...
> 
> Okay. :-)

I hate to be that guy, but I've just noticed that elsewhere we use URLs under http://telepathy.freedesktop.org/xmpp/ which actually points to documents. So you could put the proto-XEP there and use it as the capability URI maybe? Sorry :/

> > wocky_node_add_build (x,
> >   '(', "field",
> >     '@', "var", key,
> >     '(', "value",
> >       '$', val,
> >     ')',
> >   ')', NULL);
> > 
> > or is that less clear?
> 
> Well let's do it and see.

I like it. Particularly getting rid of the casts.

> > I don't really see why the metadata-specific test is the one which explicitly
> > sends no metadata. I guess it's testing edge cases…
> 
> Yes. It tests that no dataform is sent. I didn't want it to send an empty
> dataform in that case.

Ah, okay!

(In reply to comment #3)
> I hate to be that guy, but I've just noticed that elsewhere we use URLs under
> http://telepathy.freedesktop.org/xmpp/ which actually points to documents. So
> you could put the proto-XEP there and use it as the capability URI maybe? Sorry
> :/

The worst guy! Updated. I should put the proto XEP there too.

+      type="a{sas}" tp:type="Metadata" access="readwrite"

+    <tp:mapping name="Metadata" type="a{ua{sv}}">

Le whut.

I wonder if this will break if I pass

 Metadata: {
   'foo': [],
 }

(In reply to comment #5)
> +      type="a{sas}" tp:type="Metadata" access="readwrite"
> 
> +    <tp:mapping name="Metadata" type="a{ua{sv}}">
> 
> Le whut.

This'll be fixed when we depend on new enough tp-glib and we can remove this example. I'll ignore it for now (fixed in tp-spec master).

> I wonder if this will break if I pass
> 
>  Metadata: {
>    'foo': [],
>  }

I added a test. It initially didn't pass due to caps-helper stuff but it does now.

looks lush.

le merged, thanks.