[Sugar-devel] journal comments field patch (again)

[Daniel Narvaez]

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.bender at gmail.com> wrote:
> On Tue, Mar 26, 2013 at 5:28 PM, Daniel Narvaez <dwnarvaez at 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