1319052 - Implement F2 keyboard shortcut and inline renaming (cmd_rename) for all directory types in Address Book

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1319040 +++

Short of doing inline renaming (for which we don't have the manpower), can we at least have a simple way of renaming things in Address Book's directory tree using Windows standard shortcut key for renaming, F2? This is 21st century, and certain swiss-knife methods should just work...

STR

1) Select a directory in TB main address book
- an AB or LDAP Dir
- a mailing list
2) Press F2 on keyboard (Windows standard shortcut for renaming anything), expecting to rename the directory

Actual
- nothing

Expected
- something simple to rename the directory

I think the easiest, realistic implementation of this would be a one-for all rename dialogue, as we have for attachments.
Then we just pass the current name of the object (also for dynamic dialogue title: [Rename %Oldname]) into that dialogue.
Technically, probably implement a cmd_rename with F2 as key.

Comment 1

[pass2pawan]

What language is this based on?

Comment 2

[Thomas D. (:thomas8)]

(In reply to pass2pawan from comment #1)
> What language is this based on?

I'm not sure if I understand the question, but this bug is cross-language.
Dialogues will have to be translated by l10n teams.

Oh, programming language?

XUL/Javascript.
XUL is an XML flavor, if you know HTML you're covered.
XUL is also very well documented, Google will easily bring up MDC references.
Functions for getting the directory name are available.

So this needs one new .xul dialogue, and I think we can get away with putting the functionality into abcommon.js (rather than having a separate rename.js which looks overkill to me).
The <key> for the shortcut needs to go into addressbook.xhtml, then link up the function to show the dialogue.

~~https://dxr.mozilla.org/comm-central/source/~~
~~Search for: file:addressbook.xul~~

EDIT: Things have changed since this comment was originally posted long back:
XUL is all but gone (now .xhtml files), and DXR is now searchfox.org.
https://searchfox.org/comm-central/source
Search for: addressbook.xhtml

Comment 3

[Abdallah Afify]

Hello, Is this issue still active ?

Comment 4

[Thomas D. (:thomas8)]

(In reply to Abdallah Afify from comment #3)

Hello, Is this issue still active ?

Yes.

Comment 5

[Abdallah Afify]

okay cool, can i work on it ?

I have the mozilla-center code base only, so i need to clone laso comm-base right ?
sorry for the trivial queston but this is my first real contribution

Comment 6

[Thomas D. (:thomas8)]

(In reply to Abdallah Afify from comment #5)

okay cool, can i work on it ?

Welcome! :-)

I have the mozilla-center code base only, so i need to clone laso comm-base right ?
sorry for the trivial queston but this is my first real contribution

Don't worry, not trivial... yes, for this bug you'll need comm-central.

Comment 7

[Abdallah Afify]

Thank you :)

Comment 8

[Abdallah Afify]

Hello,

I am doing some trial and error and also according to https://developer.thunderbird.net/thunderbird-development/codebase-overview, I see that the changes should be in addressbook.xhtml not in the .xul file. I tried to edit the .xul file but when I run Thunderbird nothing changes.

Also, I see two abCommon.js one in mail/components/addrbook and the other is in suite/mailnews/components, Do I need to maintain both locations ? From what I understand that the suite directory contains code related to SeaMonkey project, is this why changes I made in .xul file does not appear when I open ThunderBird ?

Comment 9

[Thomas D. (:thomas8)]

(In reply to Abdallah Afify from comment #8)

I am doing some trial and error and also according to https://developer.thunderbird.net/thunderbird-development/codebase-overview, I see that the changes should be in addressbook.xhtml not in the .xul file. I tried to edit the .xul file but when I run Thunderbird nothing changes.

That's right. Things have changed since comment 2 was originally posted long back:
XUL is all but gone (now .xhtml files, but XUL syntax may still apply, use archived documentation), and DXR is now searchfox.org.
https://searchfox.org/comm-central/source
Search for: addressbook.xhtml

Also, I see two abCommon.js one in mail/components/addrbook and the other is in suite/mailnews/components, Do I need to maintain both locations ?

No need to maintain suite/SeaMonkey code.

From what I understand that the suite directory contains code related to SeaMonkey project, is this why changes I made in .xul file does not appear when I open ThunderBird ?

Yes.

Comment 10

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: Rename Attachment dialog

Alex, we have a new contributor here (first steps into TB) who is willing to implement dedicated renaming functionality for some directory types in address book (custom ABs, LDAP directories, mailing lists).

I suggest that we keep it simple (and doable), and just implement a plain vanilla Rename 'My Custom AB' dialog, much like the Rename Attachment dialog seen in the attached screenshot, or like the Properties dialog on custom address books created by user. The ux-efficiency goal here is that Windows users can just press F2 on a selected directory to rename that in a clutter-free and focused manner (and "Rename" context menu on all OS).

• Dynamic Rename dialog title:Rename {AB/LDAP/ML-name} (we already have other similar dialogs)
• For custom ABs, replace "Properties" context menu with "Rename" (as there's nothing else on properties)
• For LDAP directories, add "Rename" context menu in addition to "Properties" (as there's other properties, even properties tabs).
• For mailing lists, add "Rename" context menu in addition to "Edit List" (which is rebranded list properties).

I'd like to have your feedback early so that we don't work in vain.
What do you think?

Comment 11

[Thomas D. (:thomas8)]

Attachment: Screenshot 2: Current Custom AB Properties dialog

Here's the current Properties dialog on a custom address book, which is essentially a misnamed version of the rename dialog which we want to implement here.

Comment 12

[Thomas D. (:thomas8)]

Attachment: L D A P Properties

Abdallah, I guess you'll need to setup some Mailing Lists and an LDAP directory in your main address book so that you'll be able to look into that.

Comment 13

[Abdallah Afify]

Thank you for the clarification Thomas, I have already created a simple dialogue similar to mail list property. Right now, I am trying to understand what happens in the code when the user presses 'OK', like which peace of code gets executed to actually save name.

Also, I noticed that when I right click on 'All Address Book', then I choose 'Properties', the Address Book window hangs and I should close it and open it again from TB Mail main window, I think it is related to the on load or on drag events handlers in abMailListDialog.xhtml.

Comment 14

[Abdallah Afify]

Attachment: Rename Directory.png

This is what I got so far

Comment 15

[Thomas D. (:thomas8)]

(In reply to Abdallah Afify from comment #13)

Thank you for the clarification Thomas, I have already created a simple dialogue similar to mail list property.

Mailing list property isn't ideal as a sample, too much other stuff.
Better to create a custom AB:

• Select Personal AB
• File > New > Address Book: "Custom AB"

Right now, I am trying to understand what happens in the code when the user presses 'OK', like which peace of code gets executed to actually save name.

On your custom AB created per above, check out the properties dialogue and how it works for renaming (more advice on that in next comment).

Also, I noticed that when I right click on 'All Address Book', then I choose 'Properties', the Address Book window hangs and I should close it and open it again from TB Mail main window.

Yes, that's bug 1631238 which I filed yesterday when checking things here. For this bug, it doesn't matter much, as 'All Address Books' cannot be renamed.

I think it is related to the on load or on drag events handlers in abMailListDialog.xhtml.

No, that's unlikely, because abMailListDialog.xhtml is only for mailing lists. Any further insights wrt the hang: pls comment on the other bug.

Comment 16

[Abdallah Afify]

Okay, I will create an AP and LDAP directories for testing.

As I was searching I found a file called, abAddressBookNameDialog.xhtml, I think I can use it instead of creating a new dialog. All I have to do is to modify its js file to actually rename the directory in the database. What do you think ?

Comment 17

[Abdallah Afify]

Without any modifications from my part, I just hooked the F2 key event to display the abAddressBookNameDialog.xhtml, I tested on an LDAP and an AP directories and it works fine.

Comment 18

[Thomas D. (:thomas8)]

Here's some advice which could be helpful:
(mid-air collision with comment 16 and 17, so this comment written before seeing those)

Start TB Daily with developer tools and address book open

• consider creating a dedicated profile for testing purposes on Daily (use thunderbird.exe -p to start profile manager and create profile, e.g. 'profile.daily')
• TB start link:
  "C:\Program Files\DailyXXX\thunderbird.exe" -p profile.daily -no-remote -purgecaches -allow-downgrade -devtools -addressbook
  The last two command line arguments will conveniently start devtools and address book every time you open your Daily installation for testing.
• More info on command line switches:
  • http://kb.mozillazine.org/Command_line_arguments_-_Thunderbird
  • https://developer.mozilla.org/en-US/docs/Mozilla/Command_Line_Options

Use Devtools at runtime to find your starting point in code

• make sure devtools are running (from menu, or command line)
• split your screen, devtools left, TB right
• fire up the Thunderbird window you want to investigate, e.g.: "Custom AB Properties" dialog
• From devtools toolbar, pick "Inspector" tab
• Devtools, upper right corner, second toolbar button from the right: "Select an iframe as the currently targeted document": Select the window which you want to investigate (by name), e.g.: "Chrome://messenger/content/addressbook/abAddressBookNameDialog.xhtml
• Now that the correct window is selected in devtools, use leftmost button on devtools toolbar: "Pick an element from the page", then hover your target window: red-dotted border indicates selectable elements. E.g. hover dialog's OK button and it'll take you right to the button code in .xhtml file (layout layer) on Inspector tab.
• From layout layer (xhtml), figure out which code gets run from there. In our example here, a bit tricky, but you'll find a link embedded in the <dialog> which has a .js code file with a base name exactly matching the .xhtml layout file of the properties dialog (only extension differs).

Comment 19

[Thomas D. (:thomas8)]

(In reply to Abdallah Afify from comment #16)

Okay, I will create an AP and LDAP directories for testing.

As I was searching I found a file called, abAddressBookNameDialog.xhtml, I think I can use it instead of creating a new dialog. All I have to do is to modify its js file to actually rename the directory in the database. What do you think ?

That could work. Be aware that this dialog was only for custom AB's so you have to double check if it doesn't have side effects on other directory types like LDAP or Mailing List.

(In reply to Abdallah Afify from comment #17)

Without any modifications from my part, I just hooked the F2 key event to display the abAddressBookNameDialog.xhtml, I tested on an LDAP and an AP directories and it works fine.

Awesome! :-)
What's AP directory ? You mean custom AB = custom address book?
Does it also work for renaming a selected Mailing List?

Comment 20

[Thomas D. (:thomas8)]

When happy with your first iteration, pls attach .diff file to this bug, you can request me to look at it by setting flag: feedback?bugzilla2007

Comment 21

[Abdallah Afify]

Attachment: LDAP and AB directories.png

Sorry, I meant AB :)

Right now, it works now LDAP and AB (which I created), but for the mailing list, nothing changes when I press Ok. So I think the abAddressBookNameDialog is not made for mailing lists.

I am struggling a bit to find how the mailing list is updated in the database. I found a fucntions called editMailingListToDatabase, but It takes something called card as an argument and I am not sure What that is.

I attached the current directory structure I have, the My AB and My LDAP are renamed correclty but the mailing lists are not

Comment 22

[Abdallah Afify]

(In reply to Thomas D. from comment #20)

When happy with your first iteration, pls attach .diff file to this bug, you can request me to look at it by setting flag: feedback?bugzilla2007

How to generate it ?

Comment 23

[Thomas D. (:thomas8)]

(In reply to Abdallah Afify from comment #21)

I am struggling a bit to find how the mailing list is updated in the database. I found a fucntions called editMailingListToDatabase, but It takes something called card as an argument and I am not sure What that is.

well, card is the internal representation of a contact in the current, somewhat old address book, and mailing lists are probably internally represented as just another 'card'.

Comment 24

[Alessandro Castellani [:aleca]]

Comment on attachment 9141649 [details]
Screenshot 1: Rename Attachment dialog

Sure, this approach sounds good to me, thanks for the feedback request.
Be sure to ping Geoff for a review when this is ready as he's been refactoring the AB code recently and he knows this area better.

 Abdallah, thanks for your contribution :D

Comment 25

[Abdallah Afify]

Attachment: renameDirectory.diff

I attached a diff file but I am not sure if it is the correct format

Comment 26

[Abdallah Afify]

Thank you Alessandro for the feedback :)

right now I am working on renaming the mailing list.

Comment 27

[Abdallah Afify]

Hello,

I managed to make the renaming the mail list work as well but I have a little problem. The renaming does not appear immediately at the sidebar and I need to close and open the whole address book menu (it happens also when I use the already existing edit menu, maybe we should open a bug if it is not already open). Do you guys have any clue how to refresh this view. I see that there is some code trying to do that in the edit menu but it does not work.

Comment 28

[Abdallah Afify]

Attachment: Bug 1319052 - Add F2 key binding to rename directories and mail lists. r?darktrojan

Comment 29

[Geoff Lankow (:darktrojan)]

(In reply to Thomas D. from comment #0)

Short of doing inline renaming (for which we don't have the manpower),

I hate to tell you this after so much work, but inline renaming is easy. You implement isEditable and setCellText on the tree's view and add the editable attribute on the <tree> and <treecol> and it just works. You'd still need to make it happen on F2, and not happen on double-click (because we do other things on double-click).

Comment 30

[Abdallah Afify]

oh okay, I will do that.

As for the F2, I already submitted a patch and put you as a reviewer.

Also, I made an hg pull and the code does not compile any more ? do you guys have this issue or did I broke something ?

Comment 31

[Geoff Lankow (:darktrojan)]

You'll need to do hg pull on your mozilla-central tree as well. Your patch is off to a good start, just modify the F2 handling to call startEditing on the tree instead of opening windows.

Comment 32

[Abdallah Afify]

I did pull the mozilla-central as well and still not compile, I also tried compiling firefox instead of the mail app but no luck

Comment 33

[Abdallah Afify]

Sorry to bother, but I am still having this compilation problem. it is due to a missing argument in the Cookie test files. Do you have any Idea why the compiler sees the function prototype from the CookieServiceChild not the CookieService ?

Comment 34

[Geoff Lankow (:darktrojan)]

Sounds a lot like what I fixed yesterday, so is your comm-central now out-of-date? Our last automated build ran fine with c-c at dda8f7871fbbeea697ce53a0f7a45c5dced29b69 and m-c at 263963426b561b2aa687aeeaeddc4fd93fff9e57. These are probably still the most recent revisions, but that changes often.

Comment 35

[Abdallah Afify]

Yeah, I see the change you made when you removed the argument, but when I compile I have a missing argument error. I went to the function definition, I found 2, one in the CookieService and the other is in CookieServiceChild

Comment 36

[Thomas D. (:thomas8)]

Comment on attachment 9141876 [details] [diff] [review]
renameDirectory.diff

Thanks, this iteration was looking good. As per following comments from Geoff, looking forward to the next iterations.

Comment 37

[Abdallah Afify]

Thank you Thomas for the feedback

Comment 38

[Abdallah Afify]

Attachment: Bug 1319052 - Add F2 key binding to rename directories and mail lists. r?darktrojan

Comment 39

[Abdallah Afify]

Hello Geoff, I removed the popup dialog and implemented the inline editing. I created a new patch and abandon the old one because I struggled with hg amend (it is very different from git :) ).

Anyways, you are the reviewer and waiting for your feedback.

Comment 40

[Abdallah Afify]

Hello Geoff,

I have a question that is bugging me, Why did i need to put the prototype of the function "directoryNameExists" in the idl file ? why it is not visible directly from the javascript ?

Comment 41

[Geoff Lankow (:darktrojan)]

The IDL (interface definition language, or something like that) file defines the interface nsIAbManager which is what you get from MailServices.ab.
Only the attributes and functions in the interface are available, not the object itself.

We use interfaces for things like this because it is also available to C++ components.

Comment 42

[Abdallah Afify]

Okay it is clear now, thank you :).

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6a5577377e1d
Add F2 key binding to rename directories and mail lists. r=darktrojan

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e8fdfd14d556
follow-up - Fix mistake found by linting. rs=linting

Comment 45

[Thomas D. (:thomas8)]

Awesome, Abdallah, renaming address books has just become easier! Thank you for implementing my RFE.

Comment 46

[Abdallah Afify]

Thank you Thomas :)