On Tue, Mar 26, 2013 at 5:28 PM, Daniel Narvaez <dwnarv...@gmail.com> wrote: > Some initial comments on 0001
I've renamed this thread to be specific to 0001, which is adding a comments field to the journal expanded entry. Thanks for the review. > > * Did you post screenshots of this so that we can get designers approval? Yes. On the Feature page [1] is an image [2]. I had a long back and forth with Gary on the specifics a few months back. Will dig up that email thread. > * An example of the json format somewhere would be useful. The icon > property is a bit suspect (it can contain pretty different things). I agree. I broke it up into separate fields in the latest patch. An example of a comments entry in the metadata is included here: [{"message": "painting with light", "from": "Walter Bender", "icon-color": ["#ED2529", "#69BC47"]}, {"message": "more tests of comment downloads", "from": "Walter Bender", "icon": "facebook-share"}, {"message": "one more time...", "from": "Walter Bender", "icon-color": ["#69BC47", "#3C54A3"]}, {"message": "trying to trigger a signal", "from": "Walter Bender", "icon": "facebook-share"}] > * 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. done > * If you think something is kludgy please fix it before posting for review :) I had forgotten to remove that comment after I had fixed it. > * I think we can avoid the .props. everywhere, slightly nicer. I have not made this change yet, since I modeled my code on listview.py, which also uses props. I think we should have a separate patch that cleans all of this up for both modules. > * Let's use new-style pygobject signals in new code. Again, I am using the same style as in listview.py. I think we should have a separate global cleanup. > * Widgets should not show themself imo, especially as a side effect of > some unrelated method. It hurts code reusability. fixed. > * It would make the review a lot easier if the refactorings was > splitted to separate patches. Agreed. I have made 3 separate patches, although one of them is still quite large. 0001 is to stage _create_scrollable for use with multiple widget types 0002 is to pull out _write_entry as a separate method so the code can be used multiple places 0003 is the addition of the comments widget and its integration into the expanded entry (using 0001 and 0002) > In detail: > > -import simplejson I left it as simplejson for the moment as to not to add unrelated changes to this patch series. > > Let's move the whole sugar module to json while we are at it? I believe we decided to move to json in general. Should be a separate series of patches? > > - 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) Done. > 2 Allow to pass a widget_class, factor out code to DescTagsView. > (Also, why passing a widget_class instead of just a widget there?) I pass a widget now, not its class. > > - 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. In a separate patch. regards. -walter [1] http://wiki.sugarlabs.org/go/Features/Comment_box_in_journal_detail_view#UI_Design [2] http://wiki.sugarlabs.org/go/File:FB-comments.png -- Walter Bender Sugar Labs http://www.sugarlabs.org
0003-Add-comments-box-to-expanded-entry.patch
Description: Binary data
0002-Make-separate-method-for-_write_entry-so-code-can-be.patch
Description: Binary data
0001-Pass-a-widget-to-_create_scrollable-so-code-can-be-u.patch
Description: Binary data
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel