[sugar] Clipboard errata (patches)

Tomeu Vizoso tomeu
Mon Oct 20 09:33:58 EDT 2008

On Mon, Oct 20, 2008 at 3:19 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
> Thanks for the reviews so far!  While updating my jhbuild I came
> accros a couple other related patches (attached).  I'm building again
> as I write this, so I'll try to rebase all of my patches today.
> The first is just visual, the second is a change to sugar-toolkit
> which is required to support the highlighting of the tray on drag, and
> the last just makes use of the go-up/go-down arrows which have been in
> the them for some time, but never referenced in the tray code, which
> was using left/right instead.

Look good, just some nitpicks:


+        context.set_icon_pixbuf(pixbuf, pixbuf.props.width / 2,
+                                pixbuf.props.height / 2)

Named parameters can give very useful info to the casual reader
without making the code much more verbose. I prefer to write this

+        context.set_icon_pixbuf(pixbuf, hot_x=pixbuf.props.width / 2,
+                                hot_y=pixbuf.props.height / 2)


Cannot we just use gtk.Widget.drag_highlight and gtk.Widget.drag_unhighlight?


I think you can push most of the patches unless you need to do
substantial changes.



> On Mon, Oct 20, 2008 at 6:05 AM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
>> On Fri, Oct 17, 2008 at 7:06 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
>>> On Fri, Oct 17, 2008 at 12:48 PM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
>>>> On Fri, Oct 17, 2008 at 6:11 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
>>>>> This is a set of patches I worked on recently, and need to rebase on
>>>>> the latest jhbuild before I post them officially.  I wanted to expose
>>>>> them for comments before I put in that effort, since there are no
>>>>> doubt other things that will need to be changed upon review.
>>>>> Thanks!
>>>>> - Eben
>>>>> PS.  I just noticed upon attaching that the first isn't actually a
>>>>> clipboard patch, but it was part of a sprint on clipboard and
>>>>> drag'n'drop in general, so I include it.
>>>> [PATCH] Lock cursor to center of icons in favorites view on drag (#7408)
>>>> Looks good, you can as well calculate again the hot point from the
>>>> pixbuf in the context, instead of adding two private members more to
>>>> the class.
>>> Oh really?  I didn't see a way to pull that back out.  Could you give
>>> me a pointer?
>> You are right, I expected that gtk.DragContext would have an accesor
>> as well as a setter.
>>>> [PATCH] Add descriptions to clippings (#5751)
>>>> Not sure if we should do the formatting in model/clipboardobject.py,
>>>> that looks to me more like a presentation issue so should live in the
>>>> view classes?
>>> I debated this back and forth.  My decision to put it in the model
>>> meant that we could simple return a description easily in the correct
>>> format given the clipping type. Otherwise, the view needs to special
>>> case based on the type of the clipping.  It seemed to me (ignoring the
>>> details of implementation) that I really just wanted "description" to
>>> be "just a string" in the model, which the view could call up at any
>>> point.  From this perspective, it's natural for the model to return an
>>> appropriate string, and for the view to color, size, position, etc.
>>> that string as it wants, right?
>> I'm ok with the concept of a single-line description of a clipping
>> living in the model, but the _MAX_DESCRIPTION_LENGTH value seems to me
>> like a UI decision. What about moving it to be an argument of
>> get_description()?
> That sounds like a good idea.
>> +            if key == 'STRING':
>> I'm not sure all text clippings will have the STRING target, I would do instead:
>> +            if key in ['STRING', 'text/plain', ...]:
>> (I'm not really sure which are the most common targets that contain
>> text, I would check the logs after a paste, these are printed in
>> there).
> I'll take a look, thanks.  I was always unsure about the best way to
> get that info; I just needed something that worked to get the rest up
> and running.  I'll look at your previous patch to similar effect,
> also.
> - Eben
>>>> [PATCH] Prevent duplicate clippings on drag within clipboard (#8606)
>>>> Sounds good as a temporary measure, but I think that Gtk+ has support
>>>> in its trays for reordering elements with DnD. Or it may be some
>>>> extension to gtk+? Worth investigating.
>>> Yes, that's the long term solution.  I just wanted something to clean
>>> up the behavior to prevent confusion in the meantime.  There is a TODO
>>> somewhere in there where I mention we should be rearranging instead, I
>>> think.  It might be the case that doing it right makes the rest of
>>> this patch obsolete, but I'm not sure.
>> Sure, I think that your solution is very good in the meantime.
>> Thanks,
>> Tomeu

More information about the Sugar-devel mailing list