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.
> 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.
[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)
> @@ -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.
[...]
> + 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.
> + 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).
[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
--
http://sascha.silbe.org/
http://www.infra-silbe.de/
signature.asc
Description: PGP signature
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
