[Sugar-devel] webservices patch (again)
dwnarvaez at gmail.com
Tue Mar 26 17:28:13 EDT 2013
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:
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)
- old_file_path = os.path.join(self._metadata['mountpoint'],
- model.write(self._metadata, file_path=old_file_path,
self._update_title_sid = None
+ def _write_entry(self):
+ if self._metadata.get('mountpoint', '/') == '/':
+ model.write(self._metadata, update_mtime=False)
+ old_file_path = os.path.join(self._metadata['mountpoint'],
+ model.write(self._metadata, file_path=old_file_path,
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
More information about the Sugar-devel