[Sugar-devel] webservices patch (again)

[Daniel Narvaez]

Some initial comments on 0001

* Did you post screenshots of this so that we can get designers approval?
* An example of the json format somewhere would be useful. The icon
property is a bit suspect (it can contain pretty different things).
* There are several places where splitting up the code in blocks
(vertical space is free of charge, promised!) would help readability.
Most notably _init_model.
* If you think something is kludgy please fix it before posting for review :)
* I think we can avoid the .props. everywhere, slightly nicer.
* Let's use new-style pygobject signals in new code.
* Widgets should not show themself imo, especially as a side effect of
some unrelated method. It hurts code reusability.
* It would make the review a lot easier if the refactorings was
splitted to separate patches. In detail:

-import simplejson

Let's move the whole sugar module to json while we are at it?

-    def _create_scrollable(self, label):
+    def _create_scrollable(self, label, widget_class):

The changes to this function, with those related to it in the rest of
the code, can be split to two patches:

1 Make the label optional. (Also let's use named arguments and label=None)
2 Allow to pass a widget_class, factor out code to DescTagsView.
(Also, why passing a widget_class instead of just a widget there?)

-            if self._metadata.get('mountpoint', '/') == '/':
-                model.write(self._metadata, update_mtime=False)
-            else:
-                old_file_path = os.path.join(self._metadata['mountpoint'],
-                        model.get_file_name(old_title,
-                        self._metadata['mime_type']))
-                model.write(self._metadata, file_path=old_file_path,
-                        update_mtime=False)
+            self._write_entry()

         self._update_title_sid = None

+    def _write_entry(self):
+        if self._metadata.get('mountpoint', '/') == '/':
+            model.write(self._metadata, update_mtime=False)
+        else:
+            old_file_path = os.path.join(self._metadata['mountpoint'],
+                model.get_file_name(old_title,
+                                    self._metadata['mime_type']))
+            model.write(self._metadata, file_path=old_file_path,
+                        update_mtime=False)
+

I can't easily say if you are making any changes there. But even if
so, first factor out the code in one patch, then make the changes you
need.