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

[Walter Bender]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-comments-box-to-expanded-entry.patch
Type: application/octet-stream
Size: 7770 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20130328/2f462a88/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Make-separate-method-for-_write_entry-so-code-can-be.patch
Type: application/octet-stream
Size: 1826 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20130328/2f462a88/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Pass-a-widget-to-_create_scrollable-so-code-can-be-u.patch
Type: application/octet-stream
Size: 3548 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20130328/2f462a88/attachment-0005.obj>