Open Bug 339227 Opened 16 years ago Updated 2 years ago

Moving entries between address books using the keyboard (cut/copy/paste cards)

Categories

(MailNews Core :: Address Book, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: Jamie, Assigned: yuenhoe)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)
Build Identifier: version 2 alpha 1 (20060524)

Currently, as far as i have been able to determine, there is no way to move address cards/mailing lists between address books except by dragging and dropping with the mouse. This means that this functionality is unavailable to those who cannot use a mouse, which is an accessibility problem. A solution might be to make the copy/cut and paste commands (which are currently disabled in the address book) function on address book entries, although this may raise issues with pasting into other contexts, a topic covered by several other bugs. An alternative might be a "Move to" context menu item on address book entries allowing the user to move the selected card/list to a specified address book, similar to the "Move To" context menu item for email messages.

Reproducible: Always
Severity: enhancement → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: access
Request broadening the scope of this RFE to include SeaMonkey! Please!

Thanx,
Jim H. (aka CuriousJ)
Moving to core address book.
Assignee: mscott → nobody
Component: Address Book → MailNews: Address Book
OS: Windows XP → All
Product: Thunderbird → Core
QA Contact: address-book → addressbook
Hardware: PC → All
Version: unspecified → Trunk
Duplicate of this bug: 412240
Adding "(cut/copy/paste cards)" to the title to make it easier to find this bug when searching for bugs.
Summary: Moving entries between address books using the keyboard → Moving entries between address books using the keyboard (cut/copy/paste cards)
I recently spoke to Evan yan and he said he would take a look at this issue.  
Depends on: 42085
Product: Core → MailNews Core
marking wanted tb 3, but no milestone until it looks like we're getting traction.
Flags: wanted-thunderbird3+
Blocks: 42085
No longer depends on: 42085
Any chance this one might be able to be bumped up in priority?  Currently those of us using a screen reader and can not use a mouse are completely locked out of this feature.  
I'd like to work on a patch for this abit if no one else is looking at it at the moment :)
Assignee: nobody → yuenhoe
(In reply to comment #8)
> I'd like to work on a patch for this abit if no one else is looking at it at
> the moment :)

Thanks Yuen Hoe, and good luck! :)
Built using Mark Banner's code. Tested on Linux. Works as far as I can tell.

Not sure if I did the review request right..
Attachment #382496 - Flags: superreview?(dmose)
Attachment #382496 - Flags: review?(bugzilla)
Attachment #382496 - Flags: ui-review?(clarkbw)
Attachment #382496 - Flags: review?(bugzilla)
Attachment #382496 - Flags: review-
Comment on attachment 382496 [details] [diff] [review]
Patch to allow copy/cut/paste with keyboard or through menu

Thanks for taking this bug on. This patch is a great start though I think we need a few tweaks. There's some ui behaviour issues here, so I'm bringing our user experience expert in as well (Bryan).

I've just been running with the patch for a bit, here's a few comments:

- I like the fact we can copy and paste email addresses into emails, and it enables it from the contacts sidebar in the compose window which is an added bonus.

- When the address book pane is selected, then you can't paste - you have to select the results pane to be able to paste. I can see this being annoying for users copying & pasting between address books. Adding cmd_paste functionality to DirPaneController should help this.

- I'm a bit worried about the cut functionality, especially as we have no undo. For instance, I can cut an address book card (by accident, for instance) and paste it into an email. Unless I paste it back into the address book, this would get lost. It would also be removed from any lists. Maybe we should just have copy/paste functionality until bug 94407 is fixed.

- Copy & paste of mailing lists breaks the existing assertion that we keep the names of mailing lists different within the same address book. I'm not quite sure what to do here. The reason we do this is to try and not confuse autocomplete with multiple lists of the same name. Maybe Bryan has some ideas here.

- It would be nice to have copy & paste on the right-click menu. Though not a requirement for this patch to be accepted.

A few comments on the code:

>+  if(abClipData == null) return;

Space after the if please, and 2-space indent the return on the next line.

>+  abClipData.forEach(
>+    function (item) {
>+      if (item.isMailList) {
>+	ab.addMailList(GetDirectoryFromURI(item.mailListURI));
>+      } else {
>+	ab.addCard(item);
>+      }

no brackets necessary for single-line if statements, please also 2-space indent the internal lines.

>+      case "cmd_cut":
>+      case "cmd_copy":
>+        if (gAbView && gAbView.selection)
>+	  return gAbView.selection.count > 0;
>+        return false;

You just need a single return, and please 2-space indent the line.
(In reply to comment #11)
Thanks for the comments. Will work on an improved patch.

> - It would be nice to have copy & paste on the right-click menu. Though not a
> requirement for this patch to be accepted.

I was actually playing with this, but wondered if it'll be redundant :P I'll add it in then.
Attached patch Improved patchSplinter Review
- Added cmd_paste to DirPaneController
- Removed 'cut' for now
- Added copy / paste to right-click menu
- Fixed the indentation problems (bad editor config..)

Not sure how to deal with the duplicate mailing list names. Let me know if there are any more refinements I should make :)
Attachment #382496 - Attachment is obsolete: true
Attachment #384072 - Flags: ui-review?(clarkbw)
Attachment #384072 - Flags: superreview?(bienvenu)
Attachment #384072 - Flags: review?(bugzilla)
Attachment #382496 - Flags: ui-review?(clarkbw)
Attachment #382496 - Flags: superreview?(dmose)
Comment on attachment 384072 [details] [diff] [review]
Improved patch

Thanks for updating the patch.

>+var abClipData = null;

Sorry, I didn't notice this before. There are a couple of problems with having a global object here. Mainly the issue is that cross-window copy & paste won't work correctly, e.g. from the contacts sidebar of the compose window to the address book, or potentially from the results pane of the advanced address book search to the address book.

Drag and drop stores the selected rows in the clipboard, however that doesn't work cross-window for similar reasons - on the window you're dragging into, it doesn't know what the selected rows were (or in which database/view).

Can you store the card objects in the clipboard instead?

>+function copyAbResultsPaneSelection()
>+{
...
>+  goUpdateCommand("cmd_paste");
>+}

This throws an exception if I invoke copy with something selected in the contacts pane of the compose window:

[Exception... "'[JavaScript Error: "goUpdateCommand is not defined" {file: "chrome://messenger/content/addressbook/abResultsPane.js" line: 404}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 81"  data: yes]

I'm a bit surprised about this, because MsgComposeWindow.js does seem to use goUpdateCommand. Although it could be that abContactsPanel.xul needs to be updated to load globalOverlay.js as well.

>+function canPasteAbItem()
>+{
>+  var length = {};
>+  try {
>+    getAbItemTransferData({}, {}, length);
>+  }
>+  catch (ex) {
>+    return false;
>+  }

Something isn't working right here. If I select a card and copy it, I can paste that card (correct). Then I go into the main mail window and select some text and copy it, I can go back to the address book window and it still allows me to paste, although it does nothing. If I try pasting in a compose window, the text has changed from the value of the card display name/email address to the text I copied.

Additionally, we should not allow paste of mailing lists into mailing lists. The address book doesn't work with nested lists (bug 40301), so we must handle that case here.

>+      // 'cut' disabled for now, possibly until it's
>+      // possible to undo deletion.
>+      // case "cmd_cut":

Can you prefix this comment by XXX and reference the bug number for undo please?
Attachment #384072 - Flags: review?(bugzilla) → review-
hey, awesome work!  this looks really good, here are my comments so far.

(In reply to comment #11)
> - I'm a bit worried about the cut functionality, especially as we have no undo.
> For instance, I can cut an address book card (by accident, for instance) and
> paste it into an email. Unless I paste it back into the address book, this
> would get lost. It would also be removed from any lists. Maybe we should just
> have copy/paste functionality until bug 94407 is fixed.

I think cut should only work when pasted into another address book.  If you cut a contact and paste into the address field of an email we should just treat that like a copy.  Some people might have hit the wrong key and I don't think there's any harm in doing a copy instead.  We shouldn't be allowing cut to work like a delete (i.e. cut should only move contact, never be able to delete it) as we have specific interactions for delete to prevent accidental deletes.  Even if we had undo / redo I don't think people would really want to cut a contact from their AB and paste it into an email; losing the contact.

> - Copy & paste of mailing lists breaks the existing assertion that we keep the
> names of mailing lists different within the same address book. I'm not quite
> sure what to do here. The reason we do this is to try and not confuse
> autocomplete with multiple lists of the same name. Maybe Bryan has some ideas
> here.

Hmm, seems like we'd have to use similar operating system file naming conventions here.  On paste we could leave the mailing list in edit mode (with the copied name left in) such that our duplicate name detection prevents people from saving without changing it.  Alternately we could append the (copy), (copy $N) string to mailing lists; not my choice as it essentially forces you to change the name anyway.

I haven't tested this out yet but assuming we work out these remaining issues you can expect a ui-r+ from me.
Ok noted, will work on improvements. I noticed that drag and drop wouldn't work cross-window too so I thought having a global is harmless since it achieves the same amount of functionality as the drag and drop code :P

By the way, this bug asks for cut/copy/paste and bug 94407 asks for undo/redo for deletion - but if we have undo/redo for deletion then it only makes sense to have undo/redo for cut/copy/paste too - in other words these two bugs should probably be connected. So can I work on single patches which address both bugs and upload them for review here?
(In reply to comment #16)
> Ok noted, will work on improvements. I noticed that drag and drop wouldn't work
> cross-window too so I thought having a global is harmless since it achieves the
> same amount of functionality as the drag and drop code :P
> 
> By the way, this bug asks for cut/copy/paste and bug 94407 asks for undo/redo
> for deletion - but if we have undo/redo for deletion then it only makes sense
> to have undo/redo for cut/copy/paste too - in other words these two bugs should
> probably be connected. So can I work on single patches which address both bugs
> and upload them for review here?

Sure - that should probably work fine, if you indicate on that other bug that it is being addressed here.
Comment on attachment 384072 [details] [diff] [review]
Improved patch

clearing sr request based on standard8's minus.
Attachment #384072 - Flags: superreview?(bienvenu)
(In reply to comment #17)
> (In reply to comment #16)
> > By the way, this bug asks for cut/copy/paste and bug 94407 asks for undo/redo
> > for deletion - but if we have undo/redo for deletion then it only makes sense
> > to have undo/redo for cut/copy/paste too - in other words these two bugs should
> > probably be connected. So can I work on single patches which address both bugs
> > and upload them for review here?
> 
> Sure - that should probably work fine, if you indicate on that other bug that
> it is being addressed here.

Emm actually, it may be simpler to keep them as two different patches. Undo/redo may take a while to get right, so it may be better to get the copy/paste part landed then do cut/undo/redo together.

From a reviewer perspective, its easier to review smaller patches, however if you think it won't get too large then I'm happy for you to do both in the same patch.
Hmmm, okay. Think I'll just focus and get this one right first of all then, and worry about undo/redo after this one has landed. Maybe once this one lands we can modify the other bug to implementing undo/redo in general instead of just undo/redo for deletion.
Depends on: 444093
Comment on attachment 384072 [details] [diff] [review]
Improved patch

setting to minus for now to clear my reviews.  I'll look for the next patch.
Attachment #384072 - Flags: ui-review?(clarkbw) → ui-review-
Duplicate of this bug: 670517
Per Phil's request, I did a regression test on SM 2.2A1 builds and discovered the ability to move contacts among various address books did not exist until the 5-7-2011 build.  Builds dated 5-6-2011 and earlier worked fine (minus the "Edit" menu issue brought up by Jens).

This was for Windows builds only but I assume the same will apply to Linux and MAC.  I hope this helps.
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2011-05-07&enddate=2011-05-09

Possibly:
Jens Hatlak — Bug 654864 - Suite changes from |Bug 652855 - De-RDF the address book|
Duplicate of this bug: 242517
Similar bugs for other UI parts:

Keyboard ways of copying/moving things using
- Ctrl+C, Ctrl+X, or
- context menu "Copy to" / "Move to":

Bug 512942 for messages (Ctrl+C, Ctrl+X only)
Bug 404955 for folders
Bug 339227 for address book items (this bug)
Bug 335783 for attaching imgs/msgs/files/anything from clipboard
Another unbelievable missing feature... Meanwhile, to make life easier, try:

  https://addons.mozilla.org/en-US/thunderbird/addon/select-address-book-text/
  https://addons.mozilla.org/en-US/thunderbird/addon/addresscontext/

They both work on SM :-)
Duplicate of this bug: 1060382
Duplicate of this bug: 864071
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.