66955 - [FEATURE] Drag&Drop from search results pane

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[laurel]

I'm logging this bug to track decisions on support for Drag & Drop capability
from the search messages results pane.

Communicator 4.x supports Drag & Drop of mail or news search results -- dragging
a news search result should implement a Copy message instead of Move message.

Comment 1

[scottputterman]

marking nsbeta1-

Comment 2

[laurel]

*** Bug 64047 has been marked as a duplicate of this bug. ***

Comment 3

[scottputterman]

reassigning to naving

Comment 4

[Michael Collette]

Just had a user point this one out to me as a major glitch for how he goes 
about sorting his mail.  If getting drag & drop support is too much before Moz 
1.0 might it be possible to at least get the same right-click support one 
might expect in the mail summary area?  A far less optimal fix for this issue, 
as it hardly replaces the ability to drag when the folder tree is complex, but 
at least it would provide something.  Especially if groups of messages could 
be filed with the right-click menu.

Comment 5

[laurel]

*** Bug 146227 has been marked as a duplicate of this bug. ***

Comment 6

[laurel]

nominating for next release -- this would be great to have

Comment 7

[Navin Gupta]

We will need to go through the copyService to make this work, handle multiple
msgs from multiple folders. 

Comment 8

[Navin Gupta]

Attachment: proposed fix + patch for copyService clean-up bug

We can resuse the messengerdnd.js to make drag and drop work here. The only
thing we need to handle is when the user selects search results from multiple
folders. So we need to group the search results by each folder. CopyService
will do that for us because it can handle multiple requests and for each
request can handle multiple sources. So I backed some of the old checkins which
were doing that. Also had to back out the way we used to do copy when it comes
from messenger.cpp. Anytime, we have to handle multiple sources, I have now
made it so that we go
through copyService and I have tested all possible scenarios (compose,imap,
local, news, search, offline -boy was that some testing! ) 

Also nsMsgDBView had a bad bug in GetURIsForSelection, was returning bogus
uri's
for results from different folders. Also searchView needs to listen to all the
db's that have returned search result because anything can be dragged and
dropped.

Comment 9

[Navin Gupta]

Cavin, David Can I get reviews ? thx.

Comment 10

[Cavin Song]

Comment on attachment 94964 [details] [diff] [review]
proposed fix + patch for copyService clean-up bug

r=caivn. Minor tabbing issue in nsMsgCopyService.cpp and nsMessenger.cpp. Does
require more testing for the changes.

Comment 11

[David :Bienvenu]

+
rv = NS_NewISupportsArray(getter_AddRefs(folderArray));
+
if(NS_FAILED(rv))
+
{
+
	return NS_ERROR_OUT_OF_MEMORY;
 }
why blow off the error from NS_NewIsupportsArray and decide it's an out of
memory error? Why not NS_ENSURE_SUCCESS(rv, rv) instead?

conversely,
     copyRequest = new nsCopyRequest();
-    if (!copyRequest) return rv;
+    if (!copyRequest) 
+      return rv;
+
maybe this should be return NS_ERROR_OUT_OF_MEMORY?

I'm unclear how the grouping code works - this comment confused me a little
+    // duplicate the message array so we could sort the messages by it's
+    // folder easily
it looks to me more like we're creating a separate copy request for each unique
folder that has messages to be copied, and adding the folder's message(s) to the
copy request, by making multiple passes through the msgArray.

Can you add the following comment to this code?
   for (PRUint32 i=0;i<numIndicies;i++) 
   {
     nsMsgViewIndex selectedIndex = selection.GetAt(i);
-    if (!folder)
+    if (!m_folder) // must be a cross folder view, like search results
       GetFolderForViewIndex(selectedIndex, getter_AddRefs(folder));

tell me why we don't need to send a copy complete notification for msg sending?
If it's because you've moved everything into ClearCopyState, perhaps
ClearCopyState should have a different name, like ::OnCopyCompleted() or
something, because we're doing more than just clearing our copy state, aren't we?

Comment 12

[Navin Gupta]

that code was there before and I never tried to change it. It is all working fine. 
we are not creating separate request when grouping. we are just creating
additional copySource. We will have to iterate through the array multiple times
as we group each source. we remove the elements from the array as we group them. 

I'll do minor clean-ups like name change, anything else ?

Comment 13

[David :Bienvenu]

which code was already there? I had several comments so I'm not sure which one
you're referring to. If you attach a new patch, then I'll know if we're
understanding each other.

Comment 14

[Navin Gupta]

Attachment: patch

Comment 15

[David :Bienvenu]

Comment on attachment 95311 [details] [diff] [review]
patch

sr=bienvenu

Comment 16

[David :Bienvenu]

Can you look at this soon, Cavin? I'm sure Navin is anxious to get this out of
his tree.

Comment 17

[Cavin Song]

Comment on attachment 95311 [details] [diff] [review]
patch

r=cavin.

Comment 18

[Navin Gupta]

fixed

Comment 19

[laurel]

Using aug29 commercial trunk build: win98, linux rh6.2, mac OS 10.1
Implemented and generally OK. Tested basic single and multiple selection D&D. 
Did notice some instances where empty lines displayed in search results pane
after drag & drop, not consistent. Will investigate and log separate report.