[Bug 42288] Review Chan.I.FileTransfer.Metadata

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=42288

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2011-10-31 04:39:12 PDT ---
+  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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.