Closed Bug 358093 Opened 14 years ago Closed 13 years ago

cocoa drag and drop needs to implemented promised file dragging

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: jaas, Assigned: stanshebs)

References

Details

(Keywords: dogfood, regression)

Attachments

(1 file, 3 obsolete files)

Cocoa drag and drop needs to implemented promised file dragging.
*** Bug 358968 has been marked as a duplicate of this bug. ***
Flags: blocking1.9? → blocking1.9+
Status: NEW → ASSIGNED
Assignee: joshmoz → cbarrett
Status: ASSIGNED → NEW
Assignee: cbarrett → stanshebs
Target Milestone: --- → mozilla1.9alpha5
Moving to A6.

Stan, let's get a status update in this bug if at all possible.
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
At this point, promised file creation for drags to the finder desktop is working. However, the file appears at a default location (the next icon slot) in desktop or folder window, not where the mouse drops it. The Carbon code in fx2 gets it right, so must be possible. If all else fails, I could simply send a "move" appleevent to the Finder, post-drop.

I also need to check if there are any non-Finder destinations that would be expected to work (the answer is probably no, not that many random file managers installed on Macs).
Duplicate of this bug: 383043
Attached patch Proposed patch (obsolete) — Splinter Review
This patch uses the global "draggedTransferables" as a shortcut to smuggle the transferable(s) to the Cocoa promised file callback without having to define a single-use xpcom interface.

To facilitate debugging, this patch also sets up basic nspr logging, and changes a couple printfs into log message. I also renamed an iteration variable "i" to "j" so as to not shadow an outer "i".
Attachment #269086 - Flags: review?(joshmoz)
Comment on attachment 269086 [details] [diff] [review]
Proposed patch

+    printf("promised pboard is %x\n", (int)generalPboard);
[...]
+    printf ("  to pasteboard %x\n", generalPBoard);

There are a couple dangling printfs that need to get fixed.

+#ifdef MOZ_LOGGING
+
+// make sure that logging is enabled before including prlog.h
+#define FORCE_PR_LOG
+
+#include "prlog.h"
+
+#endif

Don't put blank lines between these preprocessor statements.

+  PR_LOG(sCocoaLog, PR_LOG_ALWAYS, ("draggingExited\n"));

The proper signature is "draggingExited:" - the semicolon is important. Please add it and make sure other log statements include it if they should.

In addition to that, it'd be nice to be able to more clearly indicate what class's method is getting printed out, and what that means. Maybe something like this for a log message:

"'ChildView draggingExited:' called"

This tells me what class the method is from and tells me the point of the message - you're saying that it was called.
Attachment #269086 - Flags: review?(joshmoz) → review-
Sorry, s/semicolon/colon/ in my last comment.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
This patch probably needs to be updated due to recent major changes to our drag service.
Attached patch Improved patch (obsolete) — Splinter Review
OK, here's the patch updated per review, and to play nice with current sources.
Attachment #269086 - Attachment is obsolete: true
Attachment #271768 - Flags: review?(joshmoz)
Attached patch Improved patch ++ (obsolete) — Splinter Review
Including a log message change that moved to nsClipboard.mm .
Attachment #271768 - Attachment is obsolete: true
Attachment #271771 - Flags: review?(joshmoz)
Attachment #271768 - Flags: review?(joshmoz)
Comment on attachment 271771 [details] [diff] [review]
Improved patch ++

>+    NSString *name = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];

Looks like you're leaking this string.
Attachment #271771 - Flags: review?(joshmoz) → review-
Attached patch Patch v.4Splinter Review
Updated patch to fix string leak, and replace mysterious "jpg" with empty string (note that while a totally random string works now, Apple's semi-obscure documentation makes me worry that an interesting random string could be interpreted as "only accept files with this string as extension", let's not tempt fate.)
Attachment #272066 - Flags: review?(joshmoz)
Attachment #271771 - Attachment is obsolete: true
Attachment #272066 - Flags: superreview?(mikepinkerton)
Attachment #272066 - Flags: review?(joshmoz)
Attachment #272066 - Flags: review+
Attachment #272066 - Flags: superreview?(mikepinkerton) → superreview?(pavlov)
Attachment #272066 - Flags: superreview?(pavlov) → superreview+
fixed on trunk
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
I just noticed that

"nsISupportsArray *draggedTransferables;"

is never initialized to null and it is never null-checked before use.

Stan - if you agree that this is a problem (I'm pretty confident it is) please file a bug and patch it on the new bug.
Depends on: 389114
You need to log in before you can comment on or make changes to this bug.