[Sugar-devel] [PATCH sugar] Add duplicate functionality to the Journal and enhance copy functionality

[Simon Schampijer]

On 08/08/2011 09:42 PM, Sascha Silbe wrote:
> Excerpts from Simon Schampijer's message of Fri Jul 29 18:35:41 +0200 2011:
>> This patch adds a duplicate option to the Journal entry palette and
>> the entry detail view.
>
> You're doing two changes in functionally independent parts of the code
> and the changes are not interdependent. It would make sense to split
> them into separate patches.

Hmm, in general you are right, I agree. But as those changes are linked 
together in a way I don't think it is worth the effort splitting up the 
patches and maybe introducing new errors.

>> This will replace the keep button from the activity toolbar.
>
> "This functionality"? Or perhaps "is intended to replace"? After all
> this patch doesn't actually remove the Keep button.

Sounds good, rephrased it.

> [src/jarabe/journal/journaltoolbox.py]
>> @@ -370,19 +372,24 @@ class EntryToolbar(gtk.Toolbar):
>>           self.add(self._resume)
>>           self._resume.show()
>>
>> -        self._copy = ToolButton()
>> -
>>           client = gconf.client_get_default()
>>           color = XoColor(client.get_string('/desktop/sugar/user/color'))
>> +        self._copy = ToolButton()
>>           icon = Icon(icon_name='edit-copy', xo_color=color)
>>           self._copy.set_icon_widget(icon)
>>           icon.show()
>
> [...]
>
> Is there a reason we use set_icon_widget() instead of passing the
> keyword argument 'icon' to ToolButton()? (Mentioning this because you
> add a copy of the above code for the Duplicate button)

Our toolbutton only takes one parameter the icon-name, but we want to 
color the icon. That is why we create the icon before color it 
accordingly and use the gtk.toolbutton api to set the icon widget.

>> @@ -395,6 +402,16 @@ class EntryToolbar(gtk.Toolbar):
>>
>>       def set_metadata(self, metadata):
>>           self._metadata = metadata
>> +        color = misc.get_icon_color(self._metadata)
>> +        self._copy.get_icon_widget().props.xo_color = color
>> +        if self._metadata['mountpoint'] == '/':
>> +            self._duplicate.connect('clicked', self._duplicate_clicked_cb)
>> +            self._duplicate.show()
>> +            icon = self._duplicate.get_icon_widget()
>> +            icon.props.xo_color = color
>> +            icon.show()
>> +        else:
>> +            self._duplicate.hide()
>>           self._refresh_copy_palette()
>>           self._refresh_resume_palette()
>
> Factoring out the new code into a method _refresh_duplicate_palette()
> would fit the existing code better.

Good hint, done

> [...]
>> +        file_path = model.get_file(self._metadata['uid'])
>> +        try:
>> +            model.copy(self._metadata, '/')
>> +        except IOError, e:
>> +            logging.exception('Error while copying the entry. %s', e.strerror)
>
> If we log the entire exception, we don't need to include the error
> message in the format string.

Done.

>> +            self.emit('volume-error',
>> +                      _('Error while copying the entry. %s') % e.strerror,
>> +                      _('Error'))
>
> For consistency and robustness, please always use tuples with the
> formatting operator, even if it's just a single parameter (=>  singleton
> tuple).

Done.

> [src/jarabe/journal/palettes.py]
> [CopyMenu.__init__()]
> [...]
>> +        volume_monitor = gio.volume_monitor_get()
>> +        icon_theme = gtk.icon_theme_get_default()
>> +        for mount in volume_monitor.get_mounts():
> [...]
>
> I still don't like the code duplication, but hopefully we'll clean that
> up later.
>
> Sascha

Ack-ed with those changes?

Regards,
    Simon