1140748 - Implement fix from bug 1048791 in a better way

Status: NEW

(Thunderbird :: Account Manager, enhancement)

Description

[:aceman]

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

Comment 14 Ben Bucksch (:BenB) 2015-02-24 01:37:34 CET

> -    return !incomingOK || !outgoingOK;
> +    return incomingBad | outgoingBad;

I don't see a reason for this change, and the inversion makes it harder to read.


Comment 13 Ben Bucksch (:BenB) 2015-02-24 01:35:22 CET

Comment on attachment 8565195 [details] [diff] [review] [diff] [review]
patch v2

I don't like the bitflags. They make the code substantially harder to read as it was before, and they don't gain anything.

Comment 1

[:aceman]

The bitflags are useful so that the logic (determining what was actually wrong) does not need to be duplicated in open() when already done in needed(). Do you want those 4 variables to be booleans or coded in some easier to read way? What is your proposal?

Comment 2

[:aceman]

The needed() function could also return an array or an object indicating what was wrong.

Comment 3

[Ben Bucksch (:BenB)]

Thanks, aceman for re-visiting this.

I would have kept the incomingOK (i.e. expressing the variable in the positive way, instead of incomingBad), because that avoids double-negation-thinking (is not bad = is ok).

Furthermore, yes, I'd prefer these bitflags to be separate variables. Given that you define these constants, and they occupy memory anyways, the bitflag doesn't save anything and just makes the code more complex.

These were just minor comments about code style, though, as - I assume - the code is working just fine.

Comment 4

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #3)
> Thanks, aceman for re-visiting this.
> 
> I would have kept the incomingOK (i.e. expressing the variable in the
> positive way, instead of incomingBad), because that avoids
> double-negation-thinking (is not bad = is ok).
There is no negation of incomingBad int he 'needed' function.
On the other hand, the function is called 'needed' so going from incomingBad => needed = true seems obvious to me. IncomingOK then requires thinking in negation (but still not double-negation).

> Furthermore, yes, I'd prefer these bitflags to be separate variables. Given
> that you define these constants, and they occupy memory anyways, the bitflag
> doesn't save anything and just makes the code more complex.
OK, I'll look at that.

Comment 5

[:aceman]

Attachment: patch

So how do you like this? I think there are too many negations if we want to use the *OK variables for the positive state.

Comment 6

[Wayne Mery (:wsmwk)]

see comment 5. perhaps obsolete give recent iterations in account mgr?

Comment 7

[Ben Bucksch (:BenB)]

This still applies.

Comment 8

[Ben Bucksch (:BenB)]

Comment on attachment 8585542 [details] [diff] [review]
patch

Review of attachment 8585542 [details] [diff] [review]:
-----------------------------------------------------------------

Good change. Just some nits.

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +1723,3 @@
>      }
>  
> +    return ret;

Before returning, calculate whether everything is OK, and set `allOK` to a boolean:
```
ret.allOK = Object.keys(ret).every(name => ret[name]);
```
or maybe better:
```
ret.allOK = Object.values(ret).every(value => value);
```

@@ +1752,5 @@
>    {
>      assert(typeof(okCallback) == "function");
>      assert(typeof(cancelCallback) == "function");
>      // needed() also checks the parameters
>      var needed = this.needed(configSchema, configFilledIn);

NIT: Rename `var needed` to `var problems`, to make the code at the bottom clearer.

@@ +1753,5 @@
>      assert(typeof(okCallback) == "function");
>      assert(typeof(cancelCallback) == "function");
>      // needed() also checks the parameters
>      var needed = this.needed(configSchema, configFilledIn);
> +    if (Object.keys(needed).every(item => needed[item]) && onlyIfNeeded) {

Use `allOK`.

@@ +1758,5 @@
>        okCallback();
>        return;
>      }
>  
> +    assert(Object.keys(needed).some(item => !needed[item]),

ditto

Comment 9

[Ben Bucksch (:BenB)]

@Neil: Can you fix up the review changes and drive it to landing?