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

[Simon Schampijer]

On 05/28/2011 10:14 PM, Sascha Silbe wrote:
> 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.

Yes, that sounds about right:

http://dev.laptop.org/~erikos/copy/copy-black-and-white.png

> [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.

Yeah, can maybe be a separate patch.

> [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.

No, sorry anted to make a note for this:

If you look at 'get_file' [1], you can see that we return 'None' when 
there is not a file associated with the entry. This is the case for 
example for memorize activity entry. When we want to clone such an entry 
'get_file' will return None, but the DS expect the filepatch to be '' 
when there is no file associated with it [2].

As other places are checking if 'get_file' is returning 'None' I decided 
to handle this that way to be consistent.

[1] 
http://git.sugarlabs.org/sugar/mainline/blobs/master/src/jarabe/journal/model.py#line553
[2] 
http://git.sugarlabs.org/sugar/mainline/blobs/master/src/jarabe/journal/model.py#line626

> [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.

Ok.

> [...]
>> +        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).

Ok.

>> +        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

Ok.

>> +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].

Maybe, yes. It involves a string change though and makes backporting 
harder. Maybe we can split it in a separate patch.

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

Thanks for the review!
    Simon