[Sugar-devel] [PATCH sugar] Add copy-to option in the Journal

[Sascha Silbe]

Excerpts from Simon Schampijer's message of Wed May 25 00:53:36 +0200 2011:

> This will be part of replacing the keep button. The discussion is taking place
> at: http://lists.sugarlabs.org/archive/sugar-devel/2011-May/031316.html
> There are mockups and the implementation details.

I like the general idea. Unlike in the original mock-up, I wouldn't
colour the "Copy To" menu item itself. For consistency all "Copy To"
menus should include the storage devices (like you suggested). But those
are for the Design Team to decide.


[src/jarabe/journal/journaltoolbox.py]
[...]
>          volume_monitor = gio.volume_monitor_get()
> +        icon_theme = gtk.icon_theme_get_default()
>          for mount in volume_monitor.get_mounts():
[...]

We should add support for replacing the menu to
sugar.graphics.palette.Palette (taking care of (dis)connecting
item-inserted from/to self.__menu_item_inserted_cb) and just use
CopyMenu instead of duplicating its functionality.


[src/jarabe/journal/model.py]
> @@ -616,6 +616,8 @@ def copy(metadata, mount_point):
>      """
>      metadata = get(metadata['uid'])
>      file_path = get_file(metadata['uid'])
> +    if file_path is None:
> +        file_path = ''

I guess this part has been obsoleted by your volume-error work.


[src/jarabe/journal/palettes.py]
> +class CopyMenu(gtk.Menu):
[...]
> +    def __init__(self, metadata):
[...]
> +        client = gconf.client_get_default()
> +        color = XoColor(client.get_string('/desktop/sugar/user/color'))
> +
> +        clipboard_menu = ClipboardMenu(self._metadata)
> +        clipboard_menu.set_image(Icon(icon_name='transfer-from',
> +                                      xo_color=color,
> +                                      icon_size=gtk.ICON_SIZE_MENU))

Since the image is always the same, we can set it in
ClipboardMenu.__init__() and get rid of the repetitions (we have
multiple users of ClipboardMenu). Also the name confused me at first, we
should rename it to ClipboardMenuItem.

[...]
> +        journal_menu = VolumeMenu(self._metadata, _('Journal'), '/')
> +        journal_menu.set_image(Icon(icon_name='activity-journal',
> +                                    xo_color=color,
> +                                    icon_size=gtk.ICON_SIZE_MENU))

Dito (icon, VolumeMenuItem).

> +        volume_monitor = gio.volume_monitor_get()
> +        icon_theme = gtk.icon_theme_get_default()
> +        for mount in volume_monitor.get_mounts():
> +            if self._metadata['mountpoint'] == mount.get_root().get_path():
> +                continue
> +            volume_menu = VolumeMenu(self._metadata, mount.get_name(),
> +                                   mount.get_root().get_path())

minor indentation mismatch

> +class VolumeMenu(MenuItem):
[...]
> +    def __copy_to_volume_cb(self, menu_item, mount_point):
[...]
> +        try:
> +            model.copy(self._metadata, mount_point)
> +        except IOError, e:
> +            logging.exception('Error while copying the entry. %s', e.strerror)

There's no need to include e.strerror in the message since the full
Traceback is included in the log.

> +            self.emit('volume-error',
> +                      _('Error while copying the entry. %s') % e.strerror,
> +                      _('Error'))

Maybe:
                         _('Error while copying the entry: %s') % (e.strerror, ),


> +class ClipboardMenu(MenuItem):
[...]
> +    def __copy_to_clipboard_cb(self, menu_item):
> +        file_path = model.get_file(self._metadata['uid'])
> +        if not file_path or not os.path.exists(file_path):
> +            logging.warn('Entries without a file cannot be copied.')
> +            self.emit('volume-error',
> +                      _('Entries without a file cannot be copied.'),
> +                      _('Warning'))

I wonder whether we should say "without content" instead of "without a
file". Same for your recent patch [1] for OLPC#10798 [2].


Otherwise the patch looks good to me already. Nice work!

Sascha

[1] https://patchwork.sugarlabs.org/patch/793/
[2] https://dev.laptop.org/ticket/10798
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 494 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110528/c6b55b1c/attachment.pgp>