[sugar] Clipboard errata (patches)

[Tomeu Vizoso]

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:

0001-Size-and-position-clippings-correctly-when-dragged.patch

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

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

0001-Add-drag-active-property-to-tray-control-8604.patch

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

http://pygtk.org/docs/pygtk/class-gtkwidget.html#method-gtkwidget--drag-highlight

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

Thanks,

Tomeu

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