[Sugar-devel] [PATCH] Only add one time every object in the clipboard v3 - SL #3371

[Sascha Silbe]

Excerpts from godiard's message of 2012-04-20 16:17:46 +0200:

> This patch change the behaviour of the clipboard tray,
> every object is added only one time, if already exist,
> the already added object is selected.

It's worth mentioning that even though the entry is just selected, we
still trigger a Clipboard notification (showing a flashing icon in the
lower left corner). Manually selecting an entry in the Clipboard does
not cause this to happen.

On the implementation side, it should be mentioned that we're changing
the type of clipboard object ids from 32-bit integer to long integer,
with object ids being either a strictly increasing number (produced by
Clipboard._get_next_object_id()) or the return value of the built-in
hash() function applied to the selection data.

[...]
> v2: Select the already added object if needed, as sugested by Sasha.
> v3: Show the notification when copy a already existing object, as sugested by Gary

Please put the changelog below the marker line ("---"). BTW, my name is
Sascha, not Sasha. :)


[src/jarabe/frame/clipboard.py]
[class Clipboard]
> @@ -51,9 +53,16 @@ class Clipboard(gobject.GObject):
> -    def add_object(self, name):
> -        logging.debug('Clipboard.add_object')
> -        object_id = self._get_next_object_id()
> +    def add_object(self, name, data_hash=None):
> +        logging.debug('Clipboard.add_object hash %s', data_hash)

We should document the new parameter data_hash. What are the
requirements for this parameter? Do you expect a specific hash function
to be used? (with the current patch, it needs to be the return value of
the built-in hash() function applied to the selection data)

Similarly, you're changing the set of possible return values. In
addition to a clipboard object id, the method can now also return
None. We need to document when it returns None and what that means to
the caller.


> +        if data_hash is None:
> +            object_id = self._get_next_object_id()
> +        else:
> +            object_id = data_hash
> +        if object_id in self._objects:
> +            logging.debug('Object rejected, already exist')

This message is misleading. We don't really reject the object, rather we
select to the existing instance. So how about:

 +            logging.debug('Duplicate entry, selecting previous entry instead')


[...]
[src/jarabe/frame/clipboardicon.py]
> @@ -128,17 +128,19 @@ class ClipboardIcon(RadioToolButton):
>          # Clipboard object became complete. Make it the active one.
>          if self._current_percent < 100 and cb_object.get_percent() == 100:
>              self.props.active = True
> +            self.show_notification()
>  
> -            self._notif_icon = NotificationIcon()
> -            self._notif_icon.props.icon_name = self._icon.props.icon_name
> -            self._notif_icon.props.xo_color = \
> -                    XoColor('%s,%s' % (self._icon.props.stroke_color,
> -                                       self._icon.props.fill_color))
> -            frame = jarabe.frame.get_view()
> -            frame.add_notification(self._notif_icon,
> -                                   gtk.CORNER_BOTTOM_LEFT)
>          self._current_percent = cb_object.get_percent()
>  
> +    def show_notification(self):
> +        self._notif_icon = NotificationIcon()
> +        self._notif_icon.props.icon_name = self._icon.props.icon_name
> +        self._notif_icon.props.xo_color = \
> +                XoColor('%s,%s' % (self._icon.props.stroke_color,
> +                                   self._icon.props.fill_color))
> +        frame = jarabe.frame.get_view()
> +        frame.add_notification(self._notif_icon, gtk.CORNER_BOTTOM_LEFT)

Why don't you connect to the Clipboard.object-selected here in
ClipboardIcon, the same way it's already handling
Clipboard.object-state-changed? Routing related signals differently
(Clipboard -> ClipboardIcon for object-state-changed and Clipboard ->
ClipboardPanelWindow -> ClipboardIcon for object-selected) makes it hard
to understand and debug the code.


Sascha

-- 
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: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120423/73d9c43d/attachment.pgp>