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

[Walter Bender]

On Thu, Mar 28, 2013 at 10:03 AM, Daniel Narvaez <dwnarvaez at 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.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



-- 
Walter Bender
Sugar Labs
http://www.sugarlabs.org