478360 - Support inherited folder properties

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[Kent James (:rkent)]

There are often folder attributes where it is difficult to know if the best place to apply the attribute is globally, per server, or per folder. Rather than make that decision, allow attributes, when desired, to be inherited up the folder tree, to a server property, or to a global property.

This bug is intended to be used in the control of the application of message traits in bug 471833, but it is split off from that since this is a generic feature.

Comment 1

[Kent James (:rkent)]

Attachment: Implementation and test

Here's the patch. I am going to wait for review until I have used this in the message trait calculations, just to make sure I am happy with the overall design.

Comment 2

[Kent James (:rkent)]

Attachment: De-bit rotted, slight change to documentation

For an example of how this will be used, see the patch-in-progress that I will post soon on bug 471833.

Comment 3

[[e-mail]]

Comment on attachment 362999 [details] [diff] [review]
De-bit rotted, slight change to documentation

>+  nsCAutoString value;
Nit: don't use auto strings for getter_Copies parameters. (GetCharValue and GetStringProperty do getter_Copies internally.)

>+    // A value of empty means not defined, so check the parent recursively
It's a bit of a shame that we can't distinguish between null and "" :-(

>+      return parent->GetInheritedStringProperty(propertyName, propertyValue);
>+    }
>+    else
Nit: don't use else after return.

>+      nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      if (prefBranch)
Nit: two tests for the same thing.

>+  // We use the special value "__empty__" to represent an empty string to return.
Maybe the caller would prefer to choose the value to represent an empty string?

Comment 4

[[e-mail]]

(In reply to comment #3)
>(From update of attachment 362999 [details] [diff] [review])
>>+    // A value of empty means not defined, so check the parent recursively
>It's a bit of a shame that we can't distinguish between null and "" :-(
Or alternatively HasStringProperty and RemoveStringProperty methods.

Comment 5

[Kent James (:rkent)]

(In reply to comment #4)
> >>+    // A value of empty means not defined, so check the parent recursively
> >It's a bit of a shame that we can't distinguish between null and "" :-(
> Or alternatively HasStringProperty and RemoveStringProperty methods.

I don't have strong feelings about this, so if you do then I am happy to implement this. But I was trying to be as simple as possible, while making it easy to add these hooks for extensions in the main code. If left up to me though, I would leave it as I originally designed it ("" in properties means not defined so inherit, "__empty__" as special marker for "really use a blank and don't inherit" and no new write methods added.) My original design is very easy to add to the main code, and is also fairly easy for extension writers to use.

I will do a new patch which fixes the nits whenever we can agree on this issue.

Comment 6

[David :Bienvenu]

(In reply to comment #4)
> (In reply to comment #3)
> >(From update of attachment 362999 [details] [diff] [review])
> >>+    // A value of empty means not defined, so check the parent recursively
> >It's a bit of a shame that we can't distinguish between null and "" :-(
> Or alternatively HasStringProperty and RemoveStringProperty methods.

I like this suggestion, FWIW. I'm not crazy about a special "__empty__" marker.

Comment 7

[Kent James (:rkent)]

The basic issue is that the default, empty values for the mork-driven databases on the folder and server are "" and it is important that the default value mean inherit. So you don't get away from the special __empty__ value by adding these calls, you just wrap it. I would need to add to both the nsIMsgIncomingServer interface as well as the nsIMsgDBFolder interface to accomplish this - RemoveStringProperty (on both the folder and server) would simply remove the special property value __empty__, and HasStringProperty (on the folder) would check that value. (I might be able to use a new boolean property for this, but that would probably be even more intrusive). If you write a "" to the folder property it should set instead __empty__ (which would be dangerous, since it will affect calls that are not meant to be inherited), so I would then need to modify the basic setProperty command so that the __empty__ is set. I would also need to look around to make sure that there are no calls that are setting properties to "" when they really intend to simply remove the value.

So that's what I will do, even though I am very unenthusiastic about that approach. But I really need this landed, and I will do what it takes. But you could make my day by replying that you agree this is too much for such an unimportant edge case, and just accept the original approach.

The only other option I can see would be to make inheriting not the default - but that really reduces the value of this feature.

Comment 8

[David :Bienvenu]

I'm looking at this - Mork definitely knows if the property is set or not, vs. empty, as do prefs, so it's theoretically possible to propagate this information up. 

What's really not nice is that consumers of this code may have to know about "__empty__", if I understand you correctly. If it was strictly internal, then it would be a limited hack...

Comment 9

[David :Bienvenu]

HasStringProperty can be implemented correctly on nsIMsgFolder and nsIMsgIncomingServer w/o resorting to "__Empty__". In the case of folder prefs, you can check for an error getting the pref back. For the folder, it's a bit harder because you need to use nsIMdbRow::GetCell, which means both nsIDBFolderInfo and nsIMsgFolderCacheElement need to expose a HasProperty method. Similarly, RemoveProperty would call nsIMdbRow::CutColumn

Comment 10

[David :Bienvenu]

And although it may seem unduly painful to do it in a non-hacky way, other consumers could take advantage of the new methods...

Comment 11

[Kent James (:rkent)]

I'm trying to go down a different path. I really would rather avoid messing with the existing properties as that could lead to unexpected regressions, so I am proposing to add calls to nsIMsgFolder and nsIMsgIncomingServer for

  boolean getPropertyInheritance(in string propertyName)
  setPropertyInheritance(in string propertyName, in boolean aPropertyInheritance)

That will in turn set or clear a new mork value of (propertyname).IsInherited. My new hackiness that you may hate is I want make that a string value not a boolean, since strings are supported on the folder but booleans are not, plus I want to be able to control the default handling.

Also I  think that your description of what to do just reinforces my distaste for that other approach. That's a lot of coding for a rare edge case.

Anyway, I should have a new patch today that will be easier to understand than my rhetoric.

Comment 12

[Kent James (:rkent)]

Attachment: use get/set inheritProperty booleans

Here's my alternate approach that avoids the magic string. I changed this from Neil's proposal, as I did not want to do anything that would affect the existing string property code.

So I can live with this approach. The one thing I do not like, is that the consumer needs to be aware of the setting of this inheritProperty boolean when he sets a blank value. Whenever an empty string is set for the value, then the behaviour will depend on the setting of the inheritProperty boolean, which might have been set previously. The consumer needs to remember this.

I used a string rather than a boolean for the folder & server inheritProperty value, because I wanted to make sure that empty was TRUE, and because string setting was already supported, and booleans were not. I hope that you can live with that.

There seems to be a philosophical objection to magic strings like my previous __empty__ string, which I can't argue my way out of. I still think that the solution with the magic string is simpler and safer, but if you can't stand magic, then hopefully this solution will suit you.

Comment 13

[[e-mail]]

Comment on attachment 364587 [details] [diff] [review]
use get/set inheritProperty booleans

>+      rv = server->GetCharValue(propertyName, value);
...
>+      // If there is no parent, then check a global preference
I just realised that GetCharValue already defaults to a global preference ("mail.server.default." + propertyName) but will that suffice? I'm thinking that all folders should unconditionally call GetStringProperty and then root folders would inherit to GetCharValue which has this default built-in.

>+  // the value is "" or null (server for example clears the property to null if empty)
>+
>+  if (value.IsEmpty())
>+    propertyValue.Assign(EmptyCString());
>+  else
>+    propertyValue.Assign(value);
That's not supposed to happen, Assign() should convert null to "", but as you mentioned on IRC none of your callers actually returns null - GetCharValue shouldn't be returning null either, do you have a test case?

Comment 14

[Kent James (:rkent)]

Attachment: User existing mail.server.default. preferences, allow null returns

OK, I switched to using the mail.server.default. branch for the global as suggested by Neil. That forced me to use a different way to deal with the inherit/empty ambiguity (getting closer to Neil's original proposal), which in turn lets me support a null return to distinguish an undefined global from an empty string.

Comment 15

[[e-mail]]

Comment on attachment 364857 [details] [diff] [review]
User existing mail.server.default. preferences, allow null returns

Don't lose heart, each patch is better than the previous one!

>+nsMsgDBFolder::GetInheritedStringProperty(const char *propertyName, nsACString& propertyValue)
Completely overlooked this before, and bienvenu can correct me if I'm wrong, but I think coding style is to use the a prefix for arguments.

>+  if (!mIsServer)
>+  {
>+    GetForcePropertyEmpty(propertyName, &forceEmpty);
>+  }
>+  else
>+  {
>+    // root folders must get their values from the server
>+    GetServer(getter_AddRefs(server));
>+    if (server)
>+      server->GetForcePropertyEmpty(propertyName, &forceEmpty);
>+  }
I'm not convinced that we need to store the force empty property on the server rather than the root folder, but maybe I've overlooked a good reason?

>+    propertyValue.Assign(EmptyCString());
aPropertyValue.Truncate();

>+  if (server)
>+  {
>+    // servers will automatically inherit from the preference mail.server.default.(propertyName)
>+    rv = server->GetCharValue(propertyName, value);
Could use return server->GetCharValue(aPropertyName, aPropertyValue);
(but then won't need else after return)

Comment 16

[Kent James (:rkent)]

(In reply to comment #15)
> I'm not convinced that we need to store the force empty property on the server
> rather than the root folder, but maybe I've overlooked a good reason?

I tested GetStringProperty on the root folder, and it always fails. I assume that is because this is a dummy folder.

I'll fix your other issues.

Comment 17

[Kent James (:rkent)]

Attachment: Fixed some Neil nits

Fixed Neil's requests, except:

"I'm not convinced that we need to store the force empty property on the server
rather than the root folder, but maybe I've overlooked a good reason?"

GetStringProperty on a root folder fails, that's why I switched to the server.

Comment 18

[[e-mail]]

(In reply to comment #16)
> (In reply to comment #15)
> > I'm not convinced that we need to store the force empty property on the server
> > rather than the root folder, but maybe I've overlooked a good reason?
> I tested GetStringProperty on the root folder, and it always fails.
That's a good reason :-)

Comment 19

[Kent James (:rkent)]

Is there anything else need to do with this to help with the review? This is currently blocking some other work of mine.

Comment 20

[[e-mail]]

Sorry, the start of the week is a bad time for me, and my build wasn't working last night.

Comment 21

[David :Bienvenu]

Comment on attachment 364936 [details] [diff] [review]
Fixed some Neil nits

rv looks to be unused here:

+nsMsgDBFolder::GetInheritedStringProperty(const char *aPropertyName, nsACString& aPropertyValue)
+{
+  NS_ENSURE_ARG_POINTER(aPropertyName);
+  nsCString value;
+  nsresult rv;


can't you just do 

*_retval = value.Equals(NS_LITERAL_CSTRING("true"));

(this occurs in two places)

+  nsCString value;
+  GetStringProperty(nameEmpty.get(), value);
+  *_retval = value.Equals(NS_LITERAL_CSTRING("true")) ? PR_TRUE : PR_FALSE;

sr=me, with those things fixed, thanks!

Comment 22

[Kent James (:rkent)]

Attachment: Fixed David's nits, patch to checkin [Checkin: Comment 23]

Carrying over reviews.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 365989 [details] [diff] [review]
Fixed David's nits, patch to checkin
[Checkin: Comment 23]


http://hg.mozilla.org/comm-central/rev/938fa16bc0f0