Hi, would you mind to submit these as a pull request of
https://github.com/sugarlabs/sugar As discussed in another thread I would like to give github workflow a try. I will of course push the changes back to the official repo then. On 28 March 2013 14:26, Walter Bender <walter.ben...@gmail.com> wrote: > 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 -- Daniel Narvaez _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel