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

2011-06-08 Thread Simon Schampijer 

On 06/07/2011 09:04 PM, Sascha Silbe wrote:

Excerpts from Simon Schampijer's message of Sun May 29 15:59:56 +0200 2011:


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


Sure. But I thought we only show an error message if the user tries to
copy an entry without an associated file?

Sascha


We show an alert when you try to copy an entry without a file to an 
external device. But you are able to make a duplicate of an entry 
without a file (copy to Journal). Does that make sense?


Simon

___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

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

2011-06-08 Thread Sascha Silbe 
Excerpts from Simon Schampijer's message of Wed Jun 08 19:23:52 +0200 2011:

[copy(): map get_file() == None to file_path = '']
 We show an alert when you try to copy an entry without a file to an 
 external device. But you are able to make a duplicate of an entry 
 without a file (copy to Journal). Does that make sense?

Ah, I see. Makes perfect sense now, thanks!

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

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

2011-06-07 Thread Sascha Silbe 
Excerpts from Simon Schampijer's message of Sun May 29 15:59:56 +0200 2011:

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

Sure. But I thought we only show an error message if the user tries to
copy an entry without an associated file?

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

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

2011-05-29 Thread 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

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

2011-05-28 Thread 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/


signature.asc
Description: PGP signature
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

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

2011-05-24 Thread Simon Schampijer 
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.

Signed-off-by: Simon Schampijer si...@laptop.org
---
 src/jarabe/journal/journalactivity.py |1 +
 src/jarabe/journal/journaltoolbox.py  |   86 +++---
 src/jarabe/journal/listview.py|   13 +++
 src/jarabe/journal/model.py   |2 +
 src/jarabe/journal/palettes.py|  155 +
 5 files changed, 185 insertions(+), 72 deletions(-)

diff --git a/src/jarabe/journal/journalactivity.py 
b/src/jarabe/journal/journalactivity.py
index a33038a..bb1c7f6 100644
--- a/src/jarabe/journal/journalactivity.py
+++ b/src/jarabe/journal/journalactivity.py
@@ -171,6 +171,7 @@ class JournalActivity(JournalWindow):
 self._list_view = ListView()
 self._list_view.connect('detail-clicked', self.__detail_clicked_cb)
 self._list_view.connect('clear-clicked', self.__clear_clicked_cb)
+self._list_view.connect('volume-error', self.__volume_error_cb)
 self._main_view.pack_start(self._list_view)
 self._list_view.show()
 
diff --git a/src/jarabe/journal/journaltoolbox.py 
b/src/jarabe/journal/journaltoolbox.py
index d825bc9..18d8cdf 100644
--- a/src/jarabe/journal/journaltoolbox.py
+++ b/src/jarabe/journal/journaltoolbox.py
@@ -26,6 +26,7 @@ import gobject
 import gio
 import gtk
 
+from sugar.graphics.palette import Palette
 from sugar.graphics.toolbox import Toolbox
 from sugar.graphics.toolcombobox import ToolComboBox
 from sugar.graphics.toolbutton import ToolButton
@@ -37,11 +38,12 @@ from sugar.graphics.xocolor import XoColor
 from sugar.graphics import iconentry
 from sugar.graphics import style
 from sugar import mime
-from sugar import profile
 
 from jarabe.model import bundleregistry
 from jarabe.journal import misc
 from jarabe.journal import model
+from jarabe.journal.palettes import ClipboardMenu
+from jarabe.journal.palettes import VolumeMenu
 
 
 _AUTOSEARCH_TIMEOUT = 1000
@@ -378,7 +380,7 @@ class EntryToolbar(gtk.Toolbar):
 self._copy.set_icon_widget(icon)
 icon.show()
 
-self._copy.set_tooltip(_('Copy'))
+self._copy.set_tooltip(_('Copy to'))
 self._copy.connect('clicked', self._copy_clicked_cb)
 self.add(self._copy)
 self._copy.show()
@@ -402,19 +404,7 @@ class EntryToolbar(gtk.Toolbar):
 misc.resume(self._metadata)
 
 def _copy_clicked_cb(self, button):
-clipboard = gtk.Clipboard()
-clipboard.set_with_data([('text/uri-list', 0, 0)],
-self.__clipboard_get_func_cb,
-self.__clipboard_clear_func_cb)
-
-def __clipboard_get_func_cb(self, clipboard, selection_data, info, data):
-# Get hold of a reference so the temp file doesn't get deleted
-self._temp_file_path = model.get_file(self._metadata['uid'])
-selection_data.set_uris(['file://' + self._temp_file_path])
-
-def __clipboard_clear_func_cb(self, clipboard, data):
-# Release and delete the temp file
-self._temp_file_path = None
+button.palette.popup(immediate=True, state=Palette.SECONDARY)
 
 def _erase_button_clicked_cb(self, button):
 registry = bundleregistry.get_registry()
@@ -427,24 +417,6 @@ class EntryToolbar(gtk.Toolbar):
 def _resume_menu_item_activate_cb(self, menu_item, service_name):
 misc.resume(self._metadata, service_name)
 
-def _copy_menu_item_activate_cb(self, menu_item, mount_point):
-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'))
-return
-
-try:
-model.copy(self._metadata, mount_point)
-except IOError, e:
-logging.exception('Error while copying the entry. %s', e.strerror)
-self.emit('volume-error',
-  _('Error while copying the entry. %s') % e.strerror,
-  _('Error'))
-
 def _refresh_copy_palette(self):
 palette = self._copy.get_palette()
 
@@ -452,35 +424,43 @@ class EntryToolbar(gtk.Toolbar):
 palette.menu.remove(menu_item)
 menu_item.destroy()
 
-if self._metadata['mountpoint'] != '/':
-journal_item = MenuItem(_('Journal'))
-journal_item.set_image(Icon(
-icon_name='activity-journal',
-xo_color=profile.get_color(),
-icon_size=gtk.ICON_SIZE_MENU))
-journal_item.connect('activate',
-self._copy_menu_item_activate_cb, '/')
-journal_item.show()
-