[sugar] Clipboard errata (patches)

Mon Oct 20 17:27:11 EDT 2008

OK, I've pushed all of the patches except for "Add descriptions to
clippings", which I want to think through carefully before
resubmitting.  The changes pushed take into account the suggestions
here and discussed in IRC, where Tomeu unofficially r-plussed them.

I'll take a closer look into adding descriptions tomorrow; Tomeu's
effort in this area is already in master, so a very close
approximation of the final behavior can already be seen.

- Eben

On Mon, Oct 20, 2008 at 9:33 AM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
> 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
>>>
>>
>