Change nsIAbCard::EditCardToDatabase to nsIAbDirectory::ModifyCard

RESOLVED FIXED

Status

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

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 245891 [details] [diff] [review]
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.
(Assignee)

Comment 2

12 years ago
Created attachment 245896 [details] [diff] [review]
Patch v2

v2 patch hopefully fixes outlook compilation and bug in tb code when editing cards.
Attachment #245891 - Attachment is obsolete: true
(Assignee)

Comment 3

12 years ago
Created attachment 245946 [details] [diff] [review]
Patch v3

Further fixes for building with outlook code, thanks to Paul Tomlin for help with this.
Attachment #245896 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #245946 - Flags: superreview?(mscott)
Attachment #245946 - Flags: review?(bienvenu)

Comment 4

12 years ago
Comment on attachment 245946 [details] [diff] [review]
Patch v3

thx, Mark, looks good - do you want me to test anything?
Attachment #245946 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 5

12 years ago
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

12 years ago
Comment on attachment 245946 [details] [diff] [review]
Patch v3

sorry for the delay. Looks good.
Attachment #245946 - Flags: superreview?(mscott) → superreview+
(Assignee)

Comment 7

12 years ago
Thanks Scott.

Patch checked into trunk -> fixed.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

12 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

12 years ago
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

12 years ago
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.
(Assignee)

Comment 11

12 years ago
(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

12 years ago
(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
(Assignee)

Comment 13

12 years ago
Created attachment 247279 [details] [diff] [review]
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

12 years ago
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.
(Assignee)

Comment 15

12 years ago
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.
Attachment #247279 - Attachment is obsolete: true
(Assignee)

Comment 16

12 years ago
Created attachment 247290 [details] [diff] [review]
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

12 years ago
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.
(Assignee)

Comment 18

12 years ago
Created attachment 247322 [details] [diff] [review]
Patch v6

I reverted all the PRINTF changes. Hopefully this one will work for all cases.
Attachment #247290 - Attachment is obsolete: true

Comment 19

12 years ago
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'
(Assignee)

Comment 20

12 years ago
Created attachment 247331 [details] [diff] [review]
Patch v7

Fixes the OutlookCardMAPIProps issue. Thanks Neil.
Attachment #247322 - Attachment is obsolete: true
(Assignee)

Comment 21

12 years ago
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.
Attachment #247331 - Flags: superreview+
Attachment #247331 - Flags: review?(bienvenu)

Updated

12 years ago
Attachment #247331 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 22

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

-> Fixed.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 363993

Comment 23

12 years ago
(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

12 years ago
Created attachment 248865 [details] [diff] [review]
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
Attachment #248865 - Flags: review?(bugzilla)
(Assignee)

Comment 25

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

r=me.

Thanks for the patch.
Attachment #248865 - Flags: review?(bugzilla) → review+

Updated

12 years ago
Attachment #248865 - Flags: superreview?(bienvenu)

Comment 26

12 years ago
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.
Attachment #248865 - Flags: superreview?(bienvenu) → superreview+

Comment 27

12 years ago
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
Attachment #248865 - Attachment description: Fixed missing change to editCardToDatabase Patch v7.1 → Fixed missing change to editCardToDatabase Patch v7.1 (Checked into trunk)
Product: Core → MailNews Core
Depends on: 459956
You need to log in before you can comment on or make changes to this bug.