376238 - drag feedback should be transparent on gtk

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Neil Deakin]

The drag feedback is currently opaque because otherwise artifacts appear on the image. Need to find out why.

Comment 2

[Neil Deakin]

In reality, this is because gtk doesn't actually support translucent drag images. Alpha values are 'rounded' to 0 or 255.

So maybe this should just be disabled on linux, or just for larger images.

Comment 4

[Johnny Stenback (:jst)]

Neil, see bug 377823. This needs to be disabled in thunderbird as well, potentially conditionally on how many messages are dragged or what not.

Comment 5

[Neil Deakin]

Attachment: add a pref to disable dragimages

As an interim measure, adding a pref to disable dragimages

Comment 6

[Neil Deakin]

Checked in this patch.

Comment 7

[Dão Gottwald [:dao]]

Once this is fixed (given comment 2, will it ever be?), I think the content's background should be part of the drag feedback as well, e.g. when dragging text.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Elaborating on comment 2, gtk sorta supports translucent drag images, and there is a way to work-around the limitation.

Using the existing code-path for gtk_drag_set_icon_pixbuf (used by widget/src/gtk2/nsDragService.cpp), if the display supports alpha-blended cursors and the composition of the cursor and the icon is smaller than the maximum cursor size (64 x 64 on my system) then everything should work.  The problem is that the fall-back case creates a GTK window to hold the 'icon' image and explicitly thresholds the alpha for mask reasons.  (Another complication is that without other code changes, the mozilla code uses an rgb colormap rather than an rgba one.)

The work-around is to call gtk_drag_set_icon_widget instead, passing a GTK window of your own to hold the image.  Although this allows use of an rgba colormap and presumably that could allow extra fancy transparency, it's easier to just call gtk_window_set_opacity on the window since we only want a uniform level of transparency.  (This just sets the _NET_WM_WINDOW_OPACITY property on the X window.)

All of this, of course, requires a compositing window manager using aiglx, Xgl, or friends.

I will attach a proof-of-concept (aka badly factored) patch to this bug that implements the gtk_window_set_opacity mechanism.  Unfortunately, for reasons I do not understand (even after looking at the compiz sources), the transparency only works for me the first time I drag something, and even then intermittently.  As far as I can tell, all of the window-manager-visible attributes of the window are consistent and the actual X window is destroyed.  (But presumably I am missing something.)

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Attachment: proof-of-concept to get (unreliable) translucent dragging for gtk2 on linux

Comment 12

[Michael Ventnor]

Attachment: Patch

Nautilus managed to get this working by using pixmaps and some Cairo magic. This will make us use a pixmap with a semi-transparent pixbuf rendered on it, using similar Thebes operations as Nautilus does with Cairo.

This works beautifully, but like most things with transparent widgets, it needs a compositing window manager.

Comment 13

[Michael Ventnor]

Attachment: Patch 1.1

Oops, found a last-second leak.

Comment 14

[Michael Ventnor]

Attachment: Patch 1.2

Oops again, turns out something wasn't working like I hoped, causing an extremely serious leak :/

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    nsRefPtr<gfxContext> surfaceCtx = new gfxContext(aSurface);
+    surfaceCtx->SetOperator(gfxContext::OPERATOR_DEST_OUT);
+    surfaceCtx->Paint(0.3);
+
+    GdkPixbuf* pixbuf = nsImageToPixbuf::SurfaceToPixbuf(aSurface, dragRect.width, dragRect.height);
+    if (!pixbuf)
+      return PR_FALSE;

I think it's a bad idea to mangle aSurface and then return false. I know the alpha values aren't going to be used but probably the fallback image is darker because the premultiplied color channel values have decreased? Anyway, it's confusing. Just use a temporary surface here.

In fact, I think you can simplify this code by creating a gfxXlibSurface that wraps the pixmap. Then you can use cairo to clear it and then just draw aSurface into it with opacity 0.7. Don't need a temporary pixbuf at all.

Comment 16

[Michael Ventnor]

Attachment: Patch 2

Yeah, using an Xlib surface works. It might even be faster, depending on how GDK compares with Thebes.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

CreateAlphaPixbuf should be called SetAlphaPixmap I think. There's no pixbuf here any more.

+                                 nsRect dragRect)

const nsRect&

+    GdkVisual* visual = gdk_drawable_get_visual(GDK_DRAWABLE(pixmap));
+    Visual* XVisual = gdk_x11_visual_get_xvisual(visual);
+    Display* display = gdk_x11_drawable_get_xdisplay(GDK_DRAWABLE(pixmap));
+    Drawable drawable = gdk_x11_drawable_get_xid(GDK_DRAWABLE(pixmap));

This code is repeated in nsWindow::OnExposeEvent. We should be able to share it, perhaps via a static method in nsToolkit.cpp exposed via nsGTKToolit.h?

I'd use "xVisual" and "xPixmapSurface" and "xPixmapCtx" to conform to our naming conventions.

+    XPixmapCtx->Rectangle(gfxRect(0, 0, dragRect.width, dragRect.height));
+    XPixmapCtx->Fill();

xPixmapCtx->Paint(); --- that just fills the whole surface

+    XPixmapCtx->Paint(0.5);

0.7? Probably should make it a named #define constant at the top of the file to make it clear what this means.

Comment 18

[Michael Ventnor]

(In reply to comment #17)
> +    GdkVisual* visual = gdk_drawable_get_visual(GDK_DRAWABLE(pixmap));
> +    Visual* XVisual = gdk_x11_visual_get_xvisual(visual);
> +    Display* display = gdk_x11_drawable_get_xdisplay(GDK_DRAWABLE(pixmap));
> +    Drawable drawable = gdk_x11_drawable_get_xid(GDK_DRAWABLE(pixmap));
> 
> This code is repeated in nsWindow::OnExposeEvent. We should be able to share
> it, perhaps via a static method in nsToolkit.cpp exposed via nsGTKToolit.h?

Not possible. There are consumers of nsGTKToolkit that can't access gfx and would cause compile errors.

Also, I changed it to 0.5 because it looked better.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Put it somewhere else then. A static method on nsWindow would work. Or create nsGtkUtils.h.

Comment 20

[Michael Ventnor]

Attachment: Patch 2.1

Yeah, that works.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 325224 [details] [diff] [review]
Patch 2.1

+/* static */ already_AddRefed<gfxXlibSurface>
+nsWindow::GetXSurfaceForGdkDrawable(GdkDrawable* aDrawable,
+                                    const nsSize& aSize)

Just one quibble. Make this gfxASurface and GetSurfaceForGdkDrawable. That will make life easier for the people doing GTK-DirectFB. I think the callers don't depend on this being an X surface.

Comment 22

[Michael Ventnor]

Attachment: Patch 2.2

Comment 23

[Reed Loden [:reed]]

Pushed in changeset 654d38d83392.

Comment 24

[Reed Loden [:reed]]

Backed out due to test failures.

Comment 25

[Michael Ventnor]

Attachment: Patch 2.3

Don't use already_AddRefed<> because nsRefPtr isn't dumb like I thought. This will fix the test failures.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's best to avoid return a refcounted object with a zero refcount. So you should return already_AddRefed, but make sure that you actually add a ref.

Comment 27

[Michael Ventnor]

Attachment: Patch 2.4

Comment 28

[Dave Townsend [:mossop]]

Landed in changeset a59b394a47aa