360777 - Change nsIAbCard::EditCardToDatabase to nsIAbDirectory::ModifyCard

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

I want to move the nsIAbCard::EditCardToDatabase function from where it is to one called nsIAbDirectory::ModfiyCard. The main reason for this being it'll be easier for implementing card modifications for LDAP directories (and the comments in the idl say it should be in nsIAbDirectory as well).

The EditCardToDatabase function currently takes the uri of the directory as its argument - so any user of it currently will just have to get an instance of the directory first (which where we use it, we already have). It'll also make more sense as nsIAbDirectory already has AddCard and DeleteCard.

Comment 1

[Mark Banner (:standard8)]

Attachment: Patch v1

This patch implements what I think we need to do for this. I'm still testing the tb changes, but it shouldn't be far off.

Comment 2

[Mark Banner (:standard8)]

Attachment: Patch v2

v2 patch hopefully fixes outlook compilation and bug in tb code when editing cards.

Comment 3

[Mark Banner (:standard8)]

Attachment: Patch v3

Further fixes for building with outlook code, thanks to Paul Tomlin for help with this.

Comment 4

[David :Bienvenu]

Comment on attachment 245946 [details] [diff] [review]
Patch v3

thx, Mark, looks good - do you want me to test anything?

Comment 5

[Mark Banner (:standard8)]

Scott: ping - any chance of a sr on this patch soon? (I know you just got off holiday - it'd just I'd be useful so I can clean up my trees a little to work on other things).

(In reply to comment #4)
> (From update of attachment 245946 [details] [diff] [review] [edit])
> thx, Mark, looks good - do you want me to test anything?
> 
No need, its been compiled on Windows and I think the changes weren't significant enough to the windows parts to stop it working.

Comment 6

[Scott MacGregor]

Comment on attachment 245946 [details] [diff] [review]
Patch v3

sorry for the delay. Looks good.

Comment 7

[Mark Banner (:standard8)]

Thanks Scott.

Patch checked into trunk -> fixed.

Comment 8

[Mark Banner (:standard8)]

This broke SeaMonkey/Windows (VC8) for some really weird reason:

d:/builds/tinderbox/SeaMonkeyTrunk/WINNT_5.2_Clobber/mozilla/mailnews/addrbook/build/nsAbFactory.cpp(113) : error C2259: 'nsAbOutlookDirFactory' : cannot instantiate abstract class
d:/builds/tinderbox/SeaMonkeyTrunk/WINNT_5.2_Clobber/mozilla/mailnews/addrbook/build/nsAbFactory.cpp(123) : error C2259: 'nsAbLDAPDirFactory' : cannot instantiate abstract class

Its a clobber build and it did this for two cycles so I've got no idea where the problem lies.

Oh and a Thunderbird dep build was fine.

Any volunteers to try building this on windows for me?

Comment 9

[Paul Tomlin]

Ping me on IRC, I'll be lurking about. In the meantime I'll clean build with the patch and see if I can root out the problem.

Comment 10

[Paul Tomlin]

The result I get is


z:/MOZILLA/src/mozilla/mailnews/addrbook/build/nsAbFactory.cpp(113) : error C2259: 'nsAbOutlookDirFactory' : cannot instantiate abstract class
        due to following members:
        'nsresult nsIAbDirFactory::CreateDirectory(nsIAbDirectoryProperties *,nsISimpleEnumerator **)' : is abstract
        ../../../dist/include/addrbook\nsIAbDirFactory.h(53) : see declaration of 'nsIAbDirFactory::CreateDirectory'
z:/MOZILLA/src/mozilla/mailnews/addrbook/build/nsAbFactory.cpp(123) : error C2259: 'nsAbLDAPDirFactory' : cannot instantiate abstract class
        due to following members:
        'nsresult nsIAbDirFactory::CreateDirectory(nsIAbDirectoryProperties *,nsISimpleEnumerator **)' : is abstract
        ../../../dist/include/addrbook\nsIAbDirFactory.h(53) : see declaration of 'nsIAbDirFactory::CreateDirectory'



I'm pretty sure it must be something with the build system rather than the code, since replacing the code with a stub based on nsAbMDBDirFactory elicits the same results.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> The result I get is
...

That's good, thats what the tinderbox was coming up with.

> I'm pretty sure it must be something with the build system rather than the
> code, since replacing the code with a stub based on nsAbMDBDirFactory elicits
> the same results.

In nsAbFactory.cpp, could you try putting the #include for nsAbDirProperty.h underneath the #ifdef MOZ_LDAP_XPCOM ... #endif section?

Also, could you email me (zip them probably - normal bugzilla address) the *.h files that are located in objdir/mailnews/addrbook/public/_xpidlgen. These are generated from the .idl files, so it is probably worth me taking a quick look at them.

The only other thing I can think of is that there is something screwed up in one of the src/*.h files.

Comment 12

[[e-mail]]

(In reply to comment #11)
>In nsAbFactory.cpp, could you try putting the #include for nsAbDirProperty.h
>underneath the #ifdef MOZ_LDAP_XPCOM ... #endif section?
<windows.h> strikes again: #define CreateDirectory CreateDirectoryA
Hack 1: Copy the #undef hack from nsAbOutlookDirFactory.cpp
Hack 2: Reorder the #includes in nsAbFactory.cpp
      * Move the Outlook includes after the LDAP includes
      * Move nsAbOutlookDirFactory.h before nsAbOutlookCard.h

Comment 13

[Mark Banner (:standard8)]

Attachment: Patch v4

Revised version, this makes the OutlookCardMAPIProps struct a static member (it was already declared as static) of nsAbOutlookCard that nsAbOutlookDirectory can access. This means we can keep the nsAbWinHelper.h include in the nsAbOutlookCard.cpp file thus avoiding the CreateDirectory issues.

Paul - would you be able to test this patch for me?

If it doesn't compile, then we may need to add an include for nsAbWinHelper.h into nsAbOutlookDirectory.cpp.

Comment 14

[[e-mail]]

Comment on attachment 247279 [details] [diff] [review]
Patch v4

>+    static const PRUint32 constMaxOutlookCardMAPIProps;
>+    static const ULONG OutlookCardMAPIProps[constMaxOutlookCardMAPIProps];
VC7.1 really doesn't like these at all.
It seems to think that constMaxOutlookCardMAPIProps needs an initializer;
it doesn't know what ULONG is, because that's in windows.h;
it doesn't like static const member variables.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 247279 [details] [diff] [review]
Patch v4

(In reply to comment #14)
> (From update of attachment 247279 [details] [diff] [review] [edit])
> >+    static const PRUint32 constMaxOutlookCardMAPIProps;
> >+    static const ULONG OutlookCardMAPIProps[constMaxOutlookCardMAPIProps];
> VC7.1 really doesn't like these at all.
> It seems to think that constMaxOutlookCardMAPIProps needs an initializer;
> it doesn't know what ULONG is, because that's in windows.h;
> it doesn't like static const member variables.
> 
That's annoying. Ok, I'll have to come up with another method.

Comment 16

[Mark Banner (:standard8)]

Attachment: Patch v5

Revised version 3 but this time to re-order the headers in the module files and add a note as to why we do it.

Comment 17

[[e-mail]]

Comment on attachment 247290 [details] [diff] [review]
Patch v5

>-        PRINTF(("Huge problem URI=%s.\n", mURINoQuery)) ;
>+        PRINTF(("Huge problem URI=%s.\n", mURINoQuery))
This compiles opt but not debug; in debug PRINTF actually expands to some function call or other and needs the trailing semicolon.

Comment 18

[Mark Banner (:standard8)]

Attachment: Patch v6

I reverted all the PRINTF changes. Hopefully this one will work for all cases.

Comment 19

[[e-mail]]

Comment on attachment 247322 [details] [diff] [review]
Patch v6

nsAbOutlookDirectory.cpp(1432) : error C2039: 'OutlookCardMAPIProps' : is not a member of 'nsAbOutlookCard'
nsAbOutlookCard.h(104) : see declaration of 'nsAbOutlookCard'

Comment 20

[Mark Banner (:standard8)]

Attachment: Patch v7

Fixes the OutlookCardMAPIProps issue. Thanks Neil.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 247331 [details] [diff] [review]
Patch v7

Neil tells me this patch compiles ok on windows (thanks Neil), carrying forward Scott's sr, but checking David is still happy with the changes.

Comment 22

[Mark Banner (:standard8)]

Checked in patch v7, windows tinderboxen still green :-)

-> Fixed.

Comment 23

[[e-mail]]

(In reply to comment #22)
>Checked in patch v7, windows tinderboxen still green :-)
Unfortunately IanN checked in a patch that uses editCardToDatabase earlier
(bug 360288) which obviously no longer works - note also bug 363993.

Comment 24

[Ian Neal]

Attachment: Fixed missing change to editCardToDatabase Patch v7.1 (Checked into trunk)

This patch:
* Fixes missing change from editCardToDatabase for SM
* Incorporates fix for bug 363993 for SM

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 248865 [details] [diff] [review]
Fixed missing change to editCardToDatabase Patch v7.1 (Checked into trunk)

r=me.

Thanks for the patch.

Comment 26

[Scott MacGregor]

Comment on attachment 248865 [details] [diff] [review]
Fixed missing change to editCardToDatabase Patch v7.1 (Checked into trunk)

I can take this one for David.

Comment 27

[Ian Neal]

Comment on attachment 248865 [details] [diff] [review]
Fixed missing change to editCardToDatabase Patch v7.1 (Checked into trunk)

Checking in (trunk)
mailWindowOverlay.js;
new revision: 1.248; previous revision: 1.247
done