On Thu, Mar 28, 2013 at 10:03 AM, Daniel Narvaez <dwnarv...@gmail.com> wrote: > 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.
Done. (While I was at it, I broke 0003 into two parts, hopefully making it easier to digest.) FWIW, the process, from the requesting side, is almost identical to the merge request mechanism in gitorious. -walter > > 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 -- Walter Bender Sugar Labs http://www.sugarlabs.org _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel