Closed Bug 221651 Opened 21 years ago Closed 21 years ago

OS/2 D&D Enhancements - Phase III

Categories

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

x86
OS/2
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dragtext, Assigned: mkaply)

Details

Attachments

(2 files, 3 obsolete files)

Attached is Phase III of my proposed enhancements to OS/2 drag & drop.

It introduces a new interface, nsIDragSessionOS2, which encapsulates
support for OS/2 dragover/drop events (i.e. operations where Mozilla
is the target of a drag).  Its public methods correspond to the d&d
messages an OS/2 window receives.  Its protected methods recast native
data as nsITransferable flavors during drag-enter and drop events.

The interface, implemented by nsDragService, relieves its client,
nsWindow, of any need to "understand" drag and drop.  nsWindow simply
dispatches the messages, then performs whatever action(s) the handler
specifies.

This design also simplifies the task of avoiding video corruption
during d&d.  Virtually all of the overhead currently required to
determine if a drag is in progress has been eliminated.

Changed Files:  nsWindow (.cpp & .h) and nsDragService (.cpp & .h)
New File:  nsIDragSessionOS2.h

Note:  nsIDragSessionOS2's header belongs in mozilla/widget/public
along with those for similar platform-specific interfaces.  However,
I don't know which support files need to be modified to permit this.
Attached patch diff (obsolete) — Splinter Review
.
Assignee: blake → mkaply
Rich, this isn't compiling for me on the GCC build but I have no idea why.

g++ -o nsWidgetFactory.o -c -DOSTYPE=\"OS21\" -DOSARCH=\"OS2\"
-DHAVE_DEPENDENT_LIBS -D_IMPL_NS_WIDGET -I.
-ID:/BUILDS/current/mozilla/widget/src/os2/../xpwidgets
-ID:/BUILDS/current/mozilla/widget/src/os2 -I../../../dist/include/gfx
-I../../../dist/include/xpcom -I../../../dist/include/string
-I../../../dist/include/dom -I../../../dist/include/necko
-I../../../dist/include/uconv -I../../../dist/include/intl
-I../../../dist/include/pref -I../../../dist/include/plugin
-I../../../dist/include/webshell -I../../../dist/include/webbrowserpersist
-I../../../dist/include/widget -I../../../dist/include
-I/BUILDS/current/mozilla/obj-dbg/dist/include/nspr     -I.       -fno-rtti
-fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align
-Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-long-long -pedantic
-Zomf -pipe  -DDEBUG -D_DEBUG -DDEBUG_ -DTRACING -g -fno-inline  
-DMOZILLA_CLIENT -include ../../../mozilla-config.h -Uunix -U__unix -U__unix__
-Wp,-MD,.deps/nsWidgetFactory.pp
D:/BUILDS/current/mozilla/widget/src/os2/nsWidgetFactory.cpp
In file included from D:/BUILDS/current/mozilla/widget/src/os2/nsWindow.h:42,
                 from
D:/BUILDS/current/mozilla/widget/src/os2/nsWidgetFactory.cpp:51:
D:/BUILDS/current/mozilla/widget/src/os2/nsToolkit.h:55: warning: `class 
   nsToolkit' has virtual functions but non-virtual destructor
In file included from D:/BUILDS/current/mozilla/widget/src/os2/nsDragService.h:42,
                 from
D:/BUILDS/current/mozilla/widget/src/os2/nsWidgetFactory.cpp:52:
../../../dist/include/widget/nsIDragSessionOS2.h:123: parse error before `{' 
   token
../../../dist/include/widget/nsIDragSessionOS2.h: In static member function 
   `static const nsIID& nsIDragSessionOS2::GetIID()':
../../../dist/include/widget/nsIDragSessionOS2.h:133: parse error before `;' 
   token
../../../dist/include/string/nsBufferHandle.h: At top level:
D:/BUILDS/current/mozilla/widget/src/os2/nsToolkit.h:87: warning: `void* 
   nsToolkitWindowProc(long unsigned int, long unsigned int, void*, void*)' 
   declared `static' but never defined
make.exe[1]: *** [nsWidgetFactory.o] Error 1
make.exe[1]: Leaving directory `D:/BUILDS/current/mozilla/obj-dbg/widget/src/os2'
make: *** [all] Error 2

There is no line 123 in nsIDragSessionOS2.h....
I do not get the error that Kaply is seeing.  Instead, I get "'IDC_DNDCOUNT' was
not declared in this scope".  Rich, was your patch missing a resource file change?
Attached patch patch against trunk (obsolete) — Splinter Review
Bug 218312 was finally checked in, so I diffed against the very latest trunk.
Attachment #132905 - Attachment is obsolete: true
Rich, have a look at the patch that I attached here and make sure that I didn't
screw things up too bad.
+// return true if all items support this flavor
+//  for (PRUint32 itemIndex = 0, *_retval = PR_TRUE;
+//       itemIndex < numDragItems && *_retval; ++itemIndex) {
+//    *_retval = PR_FALSE;

Is there any reason to keep this commented-out code here?

Also, some of the lines you added in this patch are pretty long.  If possible,
could you make them wrap at 80 chars?

Otherwise, looks good to me.
mozilla/widget/public/nsIDragSessionOS2.h is misformatted:  each line break
has an extra 0x0D (i.e. CrCrLf).  This accounts for the errors Mike saw -
line 123 is a blank line in the middle of a multiline macro.  BTW... this
defective version has already been checked in - can you back it out?

> +// return true if all items support this flavor
> +//  for (PRUint32 itemIndex = 0, *_retval = PR_TRUE;
> +//       itemIndex < numDragItems && *_retval; ++itemIndex) {
> +//    *_retval = PR_FALSE;

>Is there any reason to keep this commented-out code here?

IMHO, IsDataFlavorSupported() is misimplemented.  If support for multiple-item
drags is ever added, this code will provide a more correct result.  Right now,
it doesn't matter.

Would you like a revised diff with shorter line lengths?  
Shorter line lengths per comment #8
Attachment #133887 - Attachment is obsolete: true
Attached patch updated patchSplinter Review
I added a change to mozilla/widget/public/Makefile.in, so that
nsIDragSessionOS2.h gets properly installed in the dist.  Otherwise, this patch
is exactly like the previous one.
Attachment #133983 - Attachment is obsolete: true
Attachment #132906 - Flags: review+
Comment on attachment 135509 [details] [diff] [review]
updated patch

This looks good to me.	Kaply, why don't you take a look, and if you agree,
check this in as soon as you can (before 1.6b).
Attachment #135509 - Flags: review+
We finally checked this in!

Thanks Rich :)
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: