Summary: | Drop of multiple non-image file URLs only yields one item | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Wood <dwood> | ||||||||||||||
Component: | HTML Editing | Assignee: | Dave Hyatt <hyatt> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ttalbot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 420+ | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||
Attachments: |
|
Description
Dan Wood
2005-08-29 12:01:11 PDT
Created attachment 4696 [details]
Proposed patch
If no images are found in the dragged file list we instead get a list of the
URLs of the file list (space separated) and use that.
No regression test due to difficulty testing a pasteboard containing
NSFilenamesPboardType. (Is it even possible to test that automatically?)
Comment on attachment 4696 [details]
Proposed patch
Looks like a generally good fix. But, two comments.
In TextEdit, the paths end up separated by newlines, not spaces. So it would be
better to use "\n" as the separator instead of " ", for consistency. Second,
what happens if you have a mix of image and non-image URLs? Seems to me like
with this code, you'd lose the non-image paths still.
And finally, as a matter of style, shouldn't the support for non-image types be
right in _documentFragmentWithPaths:, instead of in a separate else case? That
would also make it easier to handle a mix of image and non-image types.
Great first cut, please make these revisions and it will be read to land I
think.
Created attachment 4710 [details]
Proposed patch 2
- Moved code to _documentFragmentWithPaths
- Now places a new line between URLs
- Can include both images and other files in a single drag
Comment on attachment 4710 [details]
Proposed patch 2
For multiple lines in HTML we use p elements, not br elements.
So this code needs to be more like createFragmentFromText in markup.cpp.
Each item should be in a paragraph created by createDefaultParagraphElement.
Image items could be image elements and text items could be text nodes just as
in the code here.
A clean way to do this is to put a function over on the WebCore side that takes
a list of nodes and puts each one in a separate paragraph. The code on this
side could just pass an NSArray of DOMNode objects over the bridge to get to
that side.
Having more of the code on the WebCore side is better for portability to other
platforms anyway.
Sorry to make this harder, but we really want functions like this to be
consistent with other editing code.
Created attachment 4781 [details]
Proposed patch 3
Now generates a document fragment containing pargraphs made using
createDefaultParagraphElement in WebCore using an NSArray passed from WebKit.
Comment on attachment 4781 [details]
Proposed patch 3
This looks great! Excellent work.
A few comments:
1) Formatting nit:
+ while ((node = [nodeEnum nextObject]))
+ {
In our coding style, the brace is supposed to be on the same line as the while
statement.
2) The node should be added to the paragraph with appendChild, not with
addChild. The addChild function is for the HTML parser only.
3) Ideally, the body of this function should be inside markup.cpp -- to do
that the NSArray would have to be converted to some C++ data structure,
presumably a QPtrList<DOM::NodeImpl>. While this would make this patch more
complicated, it would reduce the amount of code that's in Mac OS X specific
Objective-C and make some other improvements practical as well (read on).
4) This function is different from createFragmentFromText in one
significant way: createFragmentFromText uses a "magic BR" at the end. I
remember precisely why that's necessary (something related to editing of
course), but I think it may be important in this case as well as that one.
Ideally, we'd refactor createFragmentFromText into two pieces, and then code to
put the nodes into paragraphs can be shared by this new function and by
createFragmentFromText. Thus createFragmentFromText would create the text nodes
and put them in a QPtrList and then call the new createFragmentfromNodeList
function.
5) The fragment returned from createDocumentFragment needs to be ref'd and
deref'd at the appropriate time; it's not legal to just call appendChild on
something newly created and floating; that could cause the fragment to be
deleted. Either use a SharedPtr<DocumentFragmentImpl> or put in explicit
ref/deref calls.
Created attachment 4814 [details]
Proposed patch 4
- Correct coding style problem with while
- Moved the actual fragment creation to markup.cpp in
createFragmentFromNodeList. Does not incorporate with createFragmentFromText
for now as createFragmentFromText doesn't create straight nodes, but uses
createParagraphContentsFromString to fill in a paragraph object that has
already been created. This means that while we have DOMNodes from
_documentFragmentWithPaths in WebKit passed across the bridge and moved to a
QPtrList<DOM::NodeImpl>, we'd have QStrings or a full paragraph (which we don't
want to wrap in another paragraph) from createParagraphContentsFromString,
creating different required code paths for the input from each routine.
- Correctly ref/deref elements and fragment while we construct the fragment.
This pattern of ref/deref matches createFragmentFromText
- Add a magic BR to the end of the fragment (this appears to be how
createFragmentFromText does it, simply appending the magic BR to the end of the
list). It looks OK in the DOM tree in Safari.
Comment on attachment 4814 [details]
Proposed patch 4
Very nice!
Two comments:
1) I feel like an idiot. Now that I read it I see that
createFragmentFromText puts each element inside a default paragraph except for
the last line if it is empty. So the magic "BR" code in
createFragmentFromNodeList is probably not needed at all. I'm really sorry
about leading you astray about this; we should probably just take that out.
2) createFragmentFromNodeList should take a const QPtrList<DOM::NodeImpl>&
rather than QPtrList<DOM::NodeImpl>*.
Despite my marking it review-, this patch is in great shape. Thanks for all the
hard work on this.
(In reply to comment #7) > - Moved the actual fragment creation to markup.cpp in > createFragmentFromNodeList. Does not incorporate with createFragmentFromText > for now as createFragmentFromText doesn't create straight nodes, but uses > createParagraphContentsFromString to fill in a paragraph object that has > already been created. This means that while we have DOMNodes from > _documentFragmentWithPaths in WebKit passed across the bridge and moved to a > QPtrList<DOM::NodeImpl>, we'd have QStrings or a full paragraph (which we don't > want to wrap in another paragraph) from createParagraphContentsFromString, > creating different required code paths for the input from each routine. The createParagraphContentsFromString function is used only inside createFragmentFromText, so it's logically part of the createFragmentFromText function. We could easily change it so that it creates a node. It can create a fragment, which is a single node that can represent a list of nodes, or the single block placeholder element. Then those nodes could be put into a QPtrList. So in the long run I do think it's practical to make createFragmentFromText use createFragmentFromNodeList. Created attachment 4825 [details]
Proposed patch 5
- Removed magic BR code
- Now takes const QPtrList<DOM::NodeImpl>& instead of QPtrList<DOM::NodeImpl>*
As to createFragmentFromText, I'm not sure what the benefit would be to create
a fragment instead of a paragraph, only to place that fragment in a paragraph
in a different routine? It would seem that this creates (possibly) a large
number of extra nodes that aren't required for what seems to be little benefit.
I will however attach an alternate patch in a moment that implements
createFragmentFromText through the new createFragmentFromNodeList.
Created attachment 4826 [details]
Proposed patch 5 alternate
Alternate version of proposed patch 5 as mentioned in comment above.
Comment on attachment 4825 [details]
Proposed patch 5
r=me
Comment on attachment 4826 [details]
Proposed patch 5 alternate
This isn't really what I had in mind as far as sharing code with the new
createFragmentFromNodeList function, so we should not land this version. We can
revisit the issue later.
|