88679 - [xlib] DND cleanup + support for dragging > 1 item

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[pocemit]

XpmCreatePixmapFromData is used to create the dnd pixmap.
It doesn't have to be colored and it doesnt have to be an XPM either. Replacing
with XCreatePixmapFromBitmapData.

Patch follows.

Comment 1

[pocemit]

Attachment: patch to switch to standard non-xpm code

Comment 2

[pocemit]

Attachment: might help if I actually remove #include <xpm.h> (final version)

Comment 3

[pocemit]

Attachment: yeah, XCreatePixmapFromBitmapData wants a char * data

Comment 4

[pocemit]

Attachment: Final removal of ALL xpm related stuff (forgot to take out -lXpm from makefile.in)

Comment 5

[Roland Mainz]

Patch looks good so far...

I wish there would be a way to leave the drag image (e.g.
-// Our drag pixmap
-static char * drag_xpm[] = {
-"32 32 5 1",
-"       c none",
-".      c #000000",
-"+      c #1142d6",
-"@      c #ffffff",
-"#      c #878787",
-"                                ",
-"                                ",
-"                                ",
-"                                ",
-"        +                       ",
-"        ++                      ",
-"        +++                     ",
-"        ++++ .............      ",
-"    +++++++++@@@@@@@@@@@@..     ",
-"     +++++++++@@@@@@@@@@@.@.    ",
-"      +++++++++@@@@@@@@@@.@@.   ",
-"       +++++++++@@@@@@@@@.@@@.  ",
-"        +++++++++@@@@@@@@.@@@@. ",
-"         +++++++++@@@@@@@.......",
-"        .#+++++++++@@@@@@@@@@@@.",
-"        .##+++++++++@@@@@@@@@@@.",
-"        .@##+++++++++@@@@@@@@@@.",
-"        .@@##++++++++@@@@@@@@@@.",
-"        .@@@##+++++++#@@@@@@@@@.",
-"        .@@@@##++++++#@@@@@@@@@.",
-"        .@@@@@##+++++#@@@@@@@@@.",
-"        .@@@@@@#######@@@@@@@@@.",
-"        .@@@@@@@######@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        .@@@@@@@@@@@@@@@@@@@@@@.",
-"        ........................"};
)
intact in the way it currrently is... it makes the source far more readable...
:-))

r=[e-mail]

Comment 6

[pocemit]

Turning this into a general xlib DND enhancement bug.
Adding support for dragging more than one item.
There is no external DND yet (from other applications) but with the following
patch it appears that all of Mozilla internal DND works fine. Patch follows.

Comment 7

[pocemit]

Attachment: latest patch to cleanup and fix DND

Comment 8

[pocemit]

gisburn: can you look over this again? I changed things a bit.
Also got rid of making extra GC - we can just use XSetWindowBackgroundPixmap to
achieve the same effect as was done with XCopyArea... And, check out that
multiple items dnd works now.

Looking for r, sr

Comment 9

[Roland Mainz]

Either remove the |#if 0 ... #endif| stuff or change it to |#ifdef
SOME_REASON_WHY_THIS_IS_DISABLED ... #endif| and you have
r=[e-mail] (new patch isn't mandatory... you
may fix that for the final patch after superreview).

Comment 10

[Christopher Blizzard (:blizzard)]

You need to fix nsDragService::IsDataFlavorSupported().

Comment 11

[pocemit]

We do support them all, because external DND is not handled (it wasn't in the
original either), and all given data types are copied directly in sequence. So
in fact, we do support any given flavor because all are copied in a loop.

Comment 12

[Christopher Blizzard (:blizzard)]

That isn't entirely true.  When you are dragging a url over mail/news folders
the folders don't support text/nsmessage.  It's asking if the source of the drag
supports a specific target flavor.

Comment 13

[pocemit]

Attachment: new version, with explanation why IsFlavorSupported is not implemented

Comment 14

[pocemit]

Okay, the way I looked at this patch is that let's first get dnd at least
somewhat working, then worry about details. this is what this does.
I don't know how to implement IsFlavorSupported, so it's a XXX FIXME type thing.
But the rest of the changes are useful. Looking for r/sr

blizzard, please?

Comment 15

[Roland Mainz]

Patch looks good and works, r=[e-mail]

Comment 16

[pocemit]

gisburn: can you test DND in mailnews, when dragging around messages?
According to blizzard, this implementation should not work there (because of
unimplemented IsFlavorSupported). If that doesn't work, it needs a separate bug
(implement IsFlavorSupported), if it does, then internal DND works in Xlib toolkit.

Comment 17

[Roland Mainz]

Sorry... it looks DnD stuff is broken in mailnews... I get the following crash
each time when I try to drag a message:
-- snip --
mailCharsetLoadListener: ISO-8859-1 
/mailnews/start.html
t@1 (l@1) signal BUS (invalid address alignment) in nsQueryInterface::operator()
at line 32 in file "nsCOMPtr.cpp"
   32                                   status = mRawPtr->QueryInterface(aIID,
answer);
(/opt/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) where
current thread: t@1
=>[1] nsQueryInterface::operator()(this = ???, aIID = STRUCT, answer = ???)
(optimized), at 0xff0e42e4 (line ~32) in "nsCOMPtr.cpp"
  [2] nsCOMPtr_base::assign_from_helper(this = ???, helper = CLASS, iid =
STRUCT) (optimized), at 0xff0e43ac (line ~65) in "nsCOMPtr.cpp"
  [3] nsCOMPtr<nsISupports>::nsCOMPtr(0xffbeb914, 0xffbeb908, 0x0, 0x12a610,
0xa72548, 0xb98018), at 0xfe165a90
  [4] XPCWrappedNative::GetNewOrUsed(ccx = CLASS, Object = ???, Scope = ???,
Interface = ???, resultWrapper = ???) (optimized), at 0xfe15e24c (line ~209) in
"xpcwrappednative.cpp"
  [5] XPCConvert::NativeInterface2JSObject(ccx = CLASS, dest = ???, src = ???,
iid = ???, scope = ???, pErr = ???) (optimized), at 0xfe13fa00 (line ~794) in
"xpcconvert.cpp"
  [6] nsXPConnect::WrapNative(this = ???, aJSContext = ???, aScope = ???,
aCOMObj = ???, aIID = STRUCT, _retval = ???) (optimized), at 0xfe12a77c (line
~491) in "nsXPConnect.cpp"
  [7] nsJSCID::CreateInstance(this = ???, _retval = ???) (optimized), at
0xfe149f5c (line ~823) in "xpcjsid.cpp"
  [8] XPTC_InvokeByIndex(0xd29db0, 0xa, 0x1, 0xffbebc70, 0x0, 0x1), at
0xff0fd470
  [9] XPCWrappedNative::CallMethod(ccx = CLASS, mode = ???) (optimized), at
0xfe16225c (line ~1880) in "xpcwrappednative.cpp"
  [10] XPC_WN_CallMethod(cx = ???, obj = ???, argc = ???, argv = ???, vp = ???)
(optimized), at 0xfe16e4b0 (line ~1253) in "xpcwrappednativejsops.cpp"
  [11] js_Invoke(cx = ???, argc = ???, flags = ???) (optimized), at 0xff244e58
(line ~807) in "jsinterp.c"
  [12] js_Interpret(cx = ???, result = ???) (optimized), at 0xff25096c (line
~2701) in "jsinterp.c"
  [13] js_Invoke(cx = ???, argc = ???, flags = ???) (optimized), at 0xff244ecc
(line ~824) in "jsinterp.c"
  [14] js_InternalInvoke(cx = ???, obj = ???, fval = ???, flags = ???, argc =
???, argv = ???, rval = ???) (optimized), at 0xff245130 (line ~896) in
"jsinterp.c"
  [15] JS_CallFunctionValue(cx = ???, obj = ???, fval = ???, argc = ???, argv =
???, rval = ???) (optimized), at 0xff21d7d4 (line ~3320) in "jsapi.c"
  [16] nsJSContext::CallEventHandler(this = ???, aTarget = ???, aHandler = ???,
argc = ???, argv = ???, aBoolResult = ???, aReverseReturnResult = ???)
(optimized), at 0xfccb36ac (line ~941) in "nsJSEnvironment.cpp"
  [17] nsJSEventListener::HandleEvent(this = ???, aEvent = ???) (optimized), at
0xfcd176c8 (line ~138) in "nsJSEventListener.cpp"
  [18] nsEventListenerManager::HandleEventSubType(this = ???, aListenerStruct =
???, aDOMEvent = ???, aCurrentTarget = ???, aSubType = ???, aPhaseFlags = ???)
(optimized), at 0xfd1724dc (line ~1161) in "nsEventListenerManager.cpp"
  [19] nsEventListenerManager::HandleEvent(this = ???, aPresContext = ???,
aEvent = ???, aDOMEvent = ???, aCurrentTarget = ???, aFlags = ???, aEventStatus
= ???) (optimized), at 0xfd174848 (line ~1976) in "nsEventListenerManager.cpp"
  [20] nsXULElement::HandleDOMEvent(this = ???, aPresContext = ???, aEvent =
???, aDOMEvent = ???, aFlags = ???, aEventStatus = ???) (optimized), at
0xfd34f71c (line ~3689) in "nsXULElement.cpp"
  [21] nsXULElement::HandleDOMEvent(this = ???, aPresContext = ???, aEvent =
???, aDOMEvent = ???, aFlags = ???, aEventStatus = ???) (optimized), at
0xfd34f800 (line ~3689) in "nsXULElement.cpp"
  [22] nsXULElement::HandleDOMEvent(this = ???, aPresContext = ???, aEvent =
???, aDOMEvent = ???, aFlags = ???, aEventStatus = ???) (optimized), at
0xfd34f800 (line ~3689) in "nsXULElement.cpp"
  [23] nsXULElement::HandleDOMEvent(this = ???, aPresContext = ???, aEvent =
???, aDOMEvent = ???, aFlags = ???, aEventStatus = ???) (optimized), at
0xfd34f800 (line ~3689) in "nsXULElement.cpp"
  [24] nsEventStateManager::GenerateDragGesture(this = ???, aPresContext = ???,
aEvent = ???) (optimized), at 0xfd17e394 (line ~1111) in
"nsEventStateManager.cpp"
  [25] nsEventStateManager::PreHandleEvent(this = ???, aPresContext = ???,
aEvent = ???, aTargetFrame = ???, aStatus = ???, aView = ???) (optimized), at
0xfd17c998 (line ~350) in "nsEventStateManager.cpp"
  [26] PresShell::HandleEventInternal(this = ???, aEvent = ???, aView = ???,
aFlags = ???, aStatus = ???) (optimized), at 0xfc59749c (line ~5657) in
"nsPresShell.cpp"
  [27] PresShell::HandleEvent(this = ???, aView = ???, aEvent = ???,
aEventStatus = ???, aForceHandle = ???, aHandled = ???) (optimized), at
0xfc5971b4 (line ~5596) in "nsPresShell.cpp"
  [28] nsView::HandleEvent(this = ???, event = ???, aEventFlags = ???, aStatus =
???, aForceHandle = ???, aHandled = ???) (optimized), at 0xfca3ebe8 (line ~364)
in "nsView.cpp"
  [29] nsView::HandleEvent(this = ???, event = ???, aEventFlags = ???, aStatus =
???, aForceHandle = ???, aHandled = ???) (optimized), at 0xfca3eb44 (line ~348)
in "nsView.cpp"
  [30] nsViewManager::DispatchEvent(this = ???, aEvent = ???, aStatus = ???)
(optimized), at 0xfca4c2c8 (line ~2050) in "nsViewManager.cpp"
  [31] HandleEvent(aEvent = ???) (optimized), at 0xfca3df6c (line ~67) in
"nsView.cpp"
  [32] nsWidget::DispatchEvent(this = 0xb43df0, aEvent = 0xffbeee54, aStatus =
nsEventStatus_eIgnore), line 1215 in "nsWidget.cpp"
  [33] nsWidget::DispatchWindowEvent(this = 0xb43df0, aEvent = STRUCT), line
1122 in "nsWidget.cpp"
  [34] nsWidget::DispatchMouseEvent(this = 0xb43df0, aEvent = STRUCT), line 1081
in "nsWidget.cpp"
  [35] nsAppShell::HandleMotionNotifyEvent(event = 0xffbeef8c, aWidget =
0xb43df0), line 712 in "nsAppShell.cpp"
  [36] nsAppShell::DispatchXEvent(event = 0xffbeef8c), line 633 in
"nsAppShell.cpp"
  [37] nsAppShell::Run(this = 0x18cea8), line 466 in "nsAppShell.cpp"
  [38] nsAppShellService::Run(this = ???) (optimized), at 0xfe0a76c4 (line ~423)
in "nsAppShellService.cpp"
  [39] main1(argc = ???, argv = ???, nativeApp = ???) (optimized), at 0x17b20
(line ~1184) in "nsAppRunner.cpp"
  [40] main(argc = ???, argv = ???) (optimized), at 0x18510 (line ~1490) in
"nsAppRunner.cpp"
-- snip --

Comment 18

[pocemit]

okay.
I never said this patch was going to be the fix of all evil etc :)
Mailnews DND can be opened under a separate bug
this is still useful (IMHO) as it gets rid of dependency on libXpm and makes
normal (non mailnews) dnd work good.

Comment 19

[Roland Mainz]

Looks like blizzard is on vacation. Adding other people for superreview...

Requesting sr= ...

Comment 20

[Christopher Blizzard (:blizzard)]

If you open another bug about the fact that IsFlavorSupported() being broken
then you can have an sr=blizzard.

Comment 21

[Roland Mainz]

Filed bug 92182 for that IsFlavorSupported() issue.

Blizzard, wanna stamp this one with your magic sr=blizzard, please ?

Comment 22

[Roland Mainz]

Per blizzard's IRC comment "if you filed the bug you're ready to go".
Thanks !!

----

CC:'ing [e-mail] for checkin, please...

Comment 23

[Mike Kaply [:mkaply]]

Fix checked in

Checking in Makefile.in;
/cvsroot/mozilla/widget/src/xlib/Makefile.in,v  <--  Makefile.in
new revision: 1.34; previous revision: 1.33
done
Checking in nsDragService.cpp;
/cvsroot/mozilla/widget/src/xlib/nsDragService.cpp,v  <--  nsDragService.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in nsDragService.h;
/cvsroot/mozilla/widget/src/xlib/nsDragService.h,v  <--  nsDragService.h
new revision: 1.3; previous revision: 1.2
done

Comment 24

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!