Hook up nsIAbDirectory::supportsMailingLists to Address Book UI

RESOLVED FIXED

Status

MailNews Core
Address Book
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: standard8, Assigned: Paul Tomlin)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

11 years ago
Currently we have in nsIAbDirectory:

readonly attribute boolean supportsMailingLists

This should be hooked up to the UI such that when this is false, then "New List" options are disabled. Should be fairly easy to do in the existing JS.

Need to block 86405 on this as ldap currently doesn't support mailing lists and I don't really want to try and work that support in until after we have got them editable.
(Assignee)

Comment 1

11 years ago
Created attachment 243764 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status

This is against mail/ I've not looked at mailnews/ because I want to see if this is what you were thinking and I'm sure more code is shared than I currently think
(Assignee)

Comment 2

11 years ago
Comment on attachment 243764 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status

ping
Attachment #243764 - Flags: review?(bugzilla)
(Reporter)

Comment 3

11 years ago
Comment on attachment 243764 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status

General note: if you do patches with -p then it tells you the function its in - useful for reviewing.

@@ -313,6 +313,7 @@
 {
   goUpdateCommand('cmd_printcard'); 
   goUpdateCommand('cmd_printcardpreview');
+  goUpdateCommand('cmd_newlist');
 }

I think the better place for this would be DirPaneSelectionChange in abCommon.js - at the moment it only updates if you open the file menu.

+          if (abDir) {
+            return abDir.supportsMailingLists;
+          }
+          return false;
+        }
+        else
+      	  return false;

This would be better as:
    if (abDir) {
      return abDir.supportsMailingLists;
    }
  }
return false;

As you're returning early  from the abDir if statement, all the other options will equate to false, hence you just need the one return false statement.

Index: mail/components/addrbook/content/addressbook.xul

You'll need to make sure you update the file menu and toolbars as well.
Attachment #243764 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 4

11 years ago
Created attachment 243975 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status

With amends as per marks suggestions.

Not sure what you mean by patch -p and this patch still doesn't touch mailnews/
Attachment #243764 - Attachment is obsolete: true
Attachment #243975 - Flags: review?(bugzilla)
(Reporter)

Updated

11 years ago
Depends on: 358647
(Reporter)

Comment 5

11 years ago
Comment on attachment 243975 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status

Thanks for the update.

Some of this doesn't work properly which isn't your fault (see bug 358647 that I raised) - the patch on their should fix it, and we'll make sure we check it in before this one.

Adding -p to the cvs diff options is the difference in the patch between:

@@ -655,6 +668,7 @@
     gPreviousDirTreeIndex = dirTree.currentIndex;
     ChangeDirectoryByURI(GetSelectedDirectory());
   }
+  goUpdateCommand('cmd_newlist');
 }


and:

@@ -655,6 +671,7 @@ function DirPaneSelectionChange()
     gPreviousDirTreeIndex = dirTree.currentIndex;
     ChangeDirectoryByURI(GetSelectedDirectory());
   }
+  goUpdateCommand('cmd_newlist');
 }

Just helps reviewers sometimes to know the function that's being changed.

-        break;       
+        break;  
+      case "cmd_newlist":
+        AbNewList();
+        break;     

After the break; there's some additional whitespace that's not required. Can you kill it please.

That leaves two problems. The first is when we bring up the address book window and the PAB is displayed, the mailing list button is disabled. We need to find how to set this correctly - I tried OnLoadDirTree in addressbook.js but that didn't fire isCommandEnabled when calling with goUpdateCommand, so I guess the command handlers may not be fully set up (?).

The second problem is when selecting the new mail list dialog, its still possible to select a directory of a type that doesn't support mailing lists. This needs some changes in the back-end code to resolve it. Are you able to do some patches that affect the c++ code? If not, it'll be relatively easy for me to patch it in as most of the structure is already there for it.

r- until we can resolve the startup of address book problem.
Attachment #243975 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> Adding -p to the cvs diff options is the difference in the patch between:
> ...
> Just helps reviewers sometimes to know the function that's being changed.

I'm using Eclipse at the moment and I can't find any way to adjust the diff switches. I'm sure it must be possible but it is currently eluding me.

> +        break;     
> 
> After the break; there's some additional whitespace that's not required. Can
> you kill it please.

Gak!

> That leaves two problems. The first is when we bring up the address book window
> and the PAB is displayed, the mailing list button is disabled. We need to find
> how to set this correctly - I tried OnLoadDirTree in addressbook.js but that
> didn't fire isCommandEnabled when calling with goUpdateCommand, so I guess the
> command handlers may not be fully set up (?).

Do you mean the platform handlers, or those in the patch? If it's the platform then it might take me some time to trace where this is all handled and what's wrong, probably easier/faster for someone who already knows this stuff.

> The second problem is when selecting the new mail list dialog, its still
> possible to select a directory of a type that doesn't support mailing lists.
> This needs some changes in the back-end code to resolve it. Are you able to do
> some patches that affect the c++ code? If not, it'll be relatively easy for me
> to patch it in as most of the structure is already there for it.

Not really, it's been many years since I done any C++ to speak of. Sorry.


Are you wanting this patch to also apply to mailnews/?
(Assignee)

Comment 7

11 years ago
Created attachment 244202 [details] [diff] [review]
add SupportsMailingLists arc to RDF source

Following a quick chat with Neil I guess this is the C++ work you mentioned. I have no idea if this is any good though.
Attachment #244202 - Flags: review?(bugzilla)
(Assignee)

Comment 8

11 years ago
Created attachment 244204 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status (3)

no r? since you're r- till the startup is fixed, which it might be here but you'll want to test yourself.

Assuming patch 244202 is OK, then this removes directories that don't support mailing lists from the new list dialog dropdown.

Add a goUpdateCommand to CommandUpdate_AddressBook in addressbook.js, which appears to work for the cmd_delete and button_delete in 2a1 during initialisation, so cmd_newlist should get an isCommandEnabled. I really must sort out a test framework here for this.

mailnews/ had a partial implementation of cmd_newlist in addressbook.xul, which was vaguely confusing, but I think it's equally hooked up as mail/ now.
Attachment #243975 - Attachment is obsolete: true
(Reporter)

Comment 9

11 years ago
Comment on attachment 244202 [details] [diff] [review]
add SupportsMailingLists arc to RDF source

 #define NC_RDF_ISREMOTE				"http://home.netscape.com/NC-rdf#IsRemote"
 #define NC_RDF_ISWRITEABLE			"http://home.netscape.com/NC-rdf#IsWriteable"
 #define NC_RDF_DIRTREENAMESORT  "http://home.netscape.com/NC-rdf#DirTreeNameSort"
+#define NC_RDF_SUPPORTSMAILINGLISTS	"http://home.netscape.com/ND-rdf#SupportsMailingLists"

In the NC_RDF_SUPPORTSMAILINGLISTS you've got ND-rdf but should have NC-rdf.

I know its not your change, but can we change all those NC_RDF_... definitions (and any above/below) so that the strings are aligned, but using spaces not tabs.

+  rv = rdf->GetResources(NS_LITERAL_CSTRING(NC_RDF_SUPPORTSMAILINGLISTS),
+                         getter_AddRefs(kNC_SupportsMailingLists));

Should be rdf->GetResource

+nsresult
+nsAbDirectoryDataSource::createDirectorySupportsMailingListsNode(nsIAbDirectory* directory,
+                                                                 nsIRDFNode **target)

You need to add this function definition into the header file as well.

+  nsresult rv;
+  PRBool supportsMailingLists;
+  rv = directory->GetSupportsMailingLists(&supportsMailingLists);

I'd prefer if you drop the first line and change the third line to nsresult rv = d...
Attachment #244202 - Flags: review?(bugzilla) → review-
(Reporter)

Comment 10

11 years ago
(In reply to comment #8)
> Created an attachment (id=244204) [edit]
> create cmd_newlist for updating enabled/disabled status (3)
> 
> no r? since you're r- till the startup is fixed, which it might be here but
> you'll want to test yourself.

I've just tested this briefly, and this patch does fix the startup problem as well.

> Assuming patch 244202 is OK, then this removes directories that don't support
> mailing lists from the new list dialog dropdown.

With the changes I've noted above I that seems to work at least on SeaMonkey (I've only tested the one so far).

So I think with appropriate updates to these two patches this is now working as I hoped. I'll review this patch fully once I've had a chance to check it on Thunderbird as well, hopefully in the next couple of days.

Thanks for all your work on these so far.
(Reporter)

Comment 11

11 years ago
(In reply to comment #10)
> I've just tested this briefly, and this patch does fix the startup problem as
> well.

I was wrong, it doesn't fix the startup problem. What I think is the problem is the ResultsPaneController doesn't have cmd_list knowledge, whereas the DirPaneController does. So I think you can copy similar code to the ResultsPaneController and drop the addressbook.js changes and it should hopefully work.
Assignee: nobody → paul
Keywords: helpwanted
(Assignee)

Comment 12

11 years ago
Created attachment 244282 [details] [diff] [review]
add SupportsMailingLists arc to RDF source (2)

> In the NC_RDF_SUPPORTSMAILINGLISTS you've got ND-rdf but should have NC-rdf.
> Should be rdf->GetResource
> You need to add this function definition into the header file as well.

how embarrassing, gak!

> I know its not your change, but can we change all those NC_RDF_... definitions
> (and any above/below) so that the strings are aligned, but using spaces not
> tabs.

done

> I'd prefer if you drop the first line and change the third line to 
> nsresult rv = d...

done, and applied to a couple of others

I've also done a little whitespace tidy up where it struck me. I get the idea that generally we try to avoid patching unrelated code, and pure formatting changes hammer the repository, but assuming I got the formatting right I don't imagine them to lead to r-
Attachment #244202 - Attachment is obsolete: true
Attachment #244282 - Flags: review?(bugzilla)
(Assignee)

Comment 13

11 years ago
Created attachment 244285 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status (4)

As per your notes. Not tested here yet, just going through setting up the win32 build prerequisites so hopefully soon I'll be able to stop posting untested patches :)
Attachment #244204 - Attachment is obsolete: true
Attachment #244285 - Flags: review?(bugzilla)
(Reporter)

Comment 14

11 years ago
(In reply to comment #12)
> I've also done a little whitespace tidy up where it struck me. I get the idea
> that generally we try to avoid patching unrelated code, and pure formatting
> changes hammer the repository, but assuming I got the formatting right I don't
> imagine them to lead to r-

Generally it's true that we try to avoid patching unrelated code, though sometimes its just sensible to tidy it up. The reason is normally for blame purposes see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsAbDirectoryQuery.cpp&rev=1.22 for instance. That gives you the most recent changes and who did them, useful for helping to track down problems and regressions.

In this case your tidy-ups look reasonable, but I did notice there were some tabs being added in (which we don't like as there is never a consistent tab space setting that everyone uses). We replace tabs as we go with spaces, the preference for new code is 2 space indentation. Sometimes though to fit in with the existing code layout 4 space is acceptable.
(Assignee)

Comment 15

11 years ago
Created attachment 244351 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status (5)

As far as I can tell this fixes all the issues noted on the bug. Initialisation is OK, drops the non-mailing-list-supporting items from the mailing list dialog, etc, etc
Attachment #244285 - Attachment is obsolete: true
Attachment #244351 - Flags: review?(bugzilla)
Attachment #244285 - Flags: review?(bugzilla)
(Reporter)

Comment 16

11 years ago
Comment on attachment 244282 [details] [diff] [review]
add SupportsMailingLists arc to RDF source (2)

+  rv = rdf->GetResource(NS_LITERAL_CSTRING(NC_RDF_SUPPORTSMAILINGLISTS),
+                         getter_AddRefs(kNC_SupportsMailingLists));

The 'g' of getter_AddRefs should be aligned with the 'N' of NS_LITERAL_CSTRING on the line above.

+      (kNC_SupportsMailingLists == property)) 

There's unnecessary whitespace on the end of this line, please remove it and check the other lines that you are changing.

+  else if ((kNC_SupportsMailingLists == property))
+      rv = createDirectorySupportsMailingListsNode(directory, target);

The second line should be two space indented.

 nsresult nsAbDirectoryDataSource::createDirectoryNameNode(nsIAbDirectory *directory,
                                                      nsIRDFNode **target)
 {
-	nsresult rv = NS_OK;
+  nsresult rv = NS_OK;
...
+  nsXPIDLString name;
+  rv = directory->GetDirName(getter_Copies(name));

Move the rv initialisation onto where we get the DirName as you've done in other places.

+	if ((kNC_Child == property))

This has a tab in it, please change to spaces. Also please check the patch for other instances of tabs (there are some) and change those to spaces as well.

r=me with the fixes noted above, thanks for the patch.
Attachment #244282 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 17

11 years ago
Created attachment 244884 [details] [diff] [review]
add SupportsMailingLists arc to RDF source (3)

with nits, minus not-mine formatting changes
Attachment #244282 - Attachment is obsolete: true
(Reporter)

Comment 18

11 years ago
Comment on attachment 244351 [details] [diff] [review]
create cmd_newlist for updating enabled/disabled status (5)

Generally this patch works and is good (and would have got r+). However, we missed one case - address book panel. This is shown in SeaMonkey under the sidebar. The context menu has a new mailing list option in it which should also be disabled as per the rules we've already defined. I think it may be just a matter of hooking up cmd_newlist to the menuitems. addressbook-panel.xul is the file to look at.

The other comments on this patch are below:

+        <rule nc:SupportsMailingLists="true"> 

There's an extra space on the end of this line that shouldn't be there.

+      case "cmd_newlist":
+        selectedDir = GetSelectedDirectory();
+        if (selectedDir) {

For the ResultsPaneController this should be var selectedDir = GetSelected...

For the isCommandEnabled of DirPaneController, please do var selectedDir at the start of the function and drop the var just after button_delete.

(Similar to the way the /mail version does it).

@@ -168,7 +182,6 @@
   isCommandEnabled: function(command)
   {
     var selectedDir;
-
     switch (command) {
       case "cmd_selectAll":

Please keep the newline - Its nicer.
Attachment #244351 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 19

11 years ago
I've been trying for a couple of days to get the cmd_newlist working OK within the addressbook-panel.xul, with some success, but there's a bit of a glitch.

addressbook-panel.xul gets

  <commandset id="addressbook-panel-commandset">
    <command id="cmd_newlist" oncommand="AbNewList();"/>
  </commandset>

with a suitable change to the menuitem.

I add

  // sidebar pane
  if (abList)
    abList.controllers.appendController(DirPaneController);

to abCommon.js:SetupAbCommandUpdateHandlers and, before LoadPreviouslySelectedAB(); in addressbook-panel.js:AbPanelLoad I add SetupAbCommandUpdateHandlers();

In effect, I think what I've done is register the DirPaneController controller on addressbook-panel.xul/[id="addressbookList"]. 

All fine well and good, but, the controller is not defined in document.commandDispatcher.getControllerForCommand("cmd_newlist") during the execution of AbPanelLoad. So isCommandEnabled doesn't get called during page initialisation (as a result of LoadPreviouslySelectedAB).

I've wrapped some dumps around the abList.controllers.appendController, with the net effect that

abList.controllers.getControllerCount() is incremented from 0 to 1
abList.controllers.getControllerForCommand("cmd_newlist") returns a valid controller
document.commandDispatcher.getControllerForCommand("cmd_newlist") is null

I'm guessing I've either not understood something, or the commandDispatcher needs time to get connected (I've tracked it to http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULCommandDispatcher.cpp#459 but then I'm lost). If the commandDispatcher needs a delay (I see a few setTimeouts floating around, I tried, it didn't help) then I'm a bit stuck about how to get around this.

Once the page is loaded and you select a different AB from the dropdown, all seems well. Just the initial enabled state may be incorrect.
(Assignee)

Comment 20

11 years ago
Created attachment 245124 [details] [diff] [review]
combined patch, with dumps for initialisation issues in addressbook-panel.xul (1)

Maybe you can see what I'm doing wrong here
Attachment #244351 - Attachment is obsolete: true
Attachment #244884 - Attachment is obsolete: true
(Assignee)

Comment 21

11 years ago
Created attachment 245159 [details] [diff] [review]
combined patch (1)

Neil suggested handling the update when the popup was showing, which solves a lot of issues. Last thing to handle is the keyboard shortcuts.
Attachment #245124 - Attachment is obsolete: true
(Assignee)

Comment 22

11 years ago
Comment on attachment 245159 [details] [diff] [review]
combined patch (1)

My mistake, there's no keyboard shortcuts on the panel. I spotted the menu access keys and got all confused.
Attachment #245159 - Attachment description: combined patch, needs keyboard shortcuts for the panel (1) → combined patch (1)
Attachment #245159 - Flags: superreview?(neil)
Attachment #245159 - Flags: review?(bugzilla)
(Reporter)

Comment 23

11 years ago
Comment on attachment 245159 [details] [diff] [review]
combined patch (1)

r=me.
Attachment #245159 - Flags: review?(bugzilla) → review+

Updated

11 years ago
Attachment #245159 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

11 years ago
Attachment #245159 - Flags: review?(mscott)

Updated

11 years ago
Attachment #245159 - Flags: review?(mscott) → review+
(Reporter)

Comment 24

11 years ago
Patch checked into trunk.

/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook-panel.xul,v  <--  addressbook-panel.xul
new revision: 1.24; previous revision: 1.23
done
Checking in mailnews/addrbook/resources/content/abMailListDialog.xul;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abMailListDialog.xul,v  <--  abMailListDialog.xul
new revision: 1.20; previous revision: 1.19
done
Checking in mailnews/addrbook/resources/content/addressbook.xul;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.xul,v  <--  addressbook.xul
new revision: 1.179; previous revision: 1.178
done
Checking in mailnews/addrbook/resources/content/abCommon.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCommon.js,v  <--  abCommon.js
new revision: 1.108; previous revision: 1.107
done
Checking in mailnews/addrbook/resources/content/addressbook-panel.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook-panel.js,v  <--  addressbook-panel.js
new revision: 1.14; previous revision: 1.13
done
Checking in mailnews/addrbook/resources/content/addressbook.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <--  addressbook.js
new revision: 1.122; previous revision: 1.121
done
Checking in mailnews/addrbook/src/nsDirectoryDataSource.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirectoryDataSource.h,v  <--  nsDirectoryDataSource.h
new revision: 1.32; previous revision: 1.31
done
Checking in mailnews/addrbook/src/nsDirectoryDataSource.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirectoryDataSource.cpp,v  <--  nsDirectoryDataSource.cpp
new revision: 1.78; previous revision: 1.77
done
Checking in mail/components/addrbook/content/abCommon.js;
/cvsroot/mozilla/mail/components/addrbook/content/abCommon.js,v  <--  abCommon.js
new revision: 1.17; previous revision: 1.16
done
Checking in mail/components/addrbook/content/addressbook.xul;
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.xul,v  <--  addressbook.xul
new revision: 1.66; previous revision: 1.65
done
Checking in mail/components/addrbook/content/addressbook.js;
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <--  addressbook.js
new revision: 1.27; previous revision: 1.26
done
Checking in mail/components/addrbook/content/abMailListDialog.xul;
/cvsroot/mozilla/mail/components/addrbook/content/abMailListDialog.xul,v  <--  abMailListDialog.xul
new revision: 1.7; previous revision: 1.6
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.