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

[Gonzalo Odiard]

On Mon, Apr 23, 2012 at 4:25 PM, Sascha Silbe <silbe at activitycentral.com>wrote:

> 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.
>
>
Yes. That behaviour was requested.


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

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


>
> [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.
>
>
>
Ok. looking at the rest of the code I thought documentation was forbidden :)




> > +        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')
>
>
>
Ok. Changed.


> [...]
> [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.
>
>
>
Ok. Changed.

I am looking at the copy of objects from the journal.
In this case, the clipboard receives a uri, but the uri is different every
time.

Gonzalo





> Sascha
>
> --
> http://sascha.silbe.org/
> http://www.infra-silbe.de/
>



-- 
Gonzalo Odiard
SugarLabs Argentina
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120423/5c94fbca/attachment.html>