Fix shadow trait notifications

[mdickinson]

This PR fixes the issue where shadow trait notifications were not being issued on the first change of the primary trait.

The essence of the change is to always call post_setattr (if it exists) when accessing a default value. That needs changes in two places:

• in trait_getattr, where there's already a block that calls post_setattr, but the call is skipped for mapped traits, for no clear reason; we re-enable that call
• in trait_setattr, where default_value_for is called if necessary to retrieve the old value for notifications. Here we add code to (1) poke the computed default value back onto the dictionary, just like we do already in trait_getattr, and (2) call post_setattr

There's scope for refactoring to remove the duplication, but I'd rather do that refactoring post-release, and do the minimum necessary to fix this bug for the release.

Note that this fix required fixing Map and PrefixMap to map a value of Undefined correctly; I think that's reasonable: mapped_value should be prepared to accept any valid value of the principal trait, and currently Undefined is such a value. (But see also #1187)

There's also a question of whether in trait_setattr we should be issuing notifications with old value Uninitialized, like we do in trait_getattr. It makes little difference in practice, since the notifiers don't call listeners when old is Uninitialized. We may want to make that change in master, for consistency, but for the bugfix I suggest leaving it out.

This builds on #1186, and will be easier to review once that PR is merged.

Fixes #1183

[kitchoi]

Is the dynamic default that depends on the mutable trait still needed or can it be removed now?

traits/traits/trait_converters.py

Lines 116 to 122 in 4e435d9

	def _mapped_trait_default(trait, name, instance):
	value = getattr(instance, name, Undefined)
	if value is Undefined:
	return Undefined
	return trait.handler.mapped_value(value)

[mdickinson]

@kitchoi It's still needed, to cope with the case where the shadow trait is accessed before the primary trait.

[mdickinson]

the case where the shadow trait is accessed before the primary trait.

Let me make sure that we do have tests covering this case ...

[mdickinson]

@kitchoi On the other hand, we can now remove that Undefined check from _mapped_trait_default.

[mdickinson]

Hmm. Travis CI seems to have gone AWOL on recent commits on this PR.

[mdickinson]

Closing and re-opening in an attempt to sort out the Travis CI status.

[kitchoi]

LGTM. Two questions.

[kitchoi]

This conversion from KeyError to AttributeError is removed. Do you know why it was there and that how it is not needed any more?
(Note: there is one like this near the bottom of setattr)

[mdickinson]

This line isn't reachable in the C code, so that conversion was never exercised (at least since the move to Python 3 only).

[mdickinson]

(This is perhaps clearer in commit 4defec0 in PR #1186.)

[kitchoi]

I see. Thank you.

[kitchoi]

Thanks! Still LGTM

[mdickinson]

Leaving this unmerged for now; I want to give it a second self-review tomorrow morning, to check that it still makes sense to me.

[kitchoi]

That's probably wise... :)

[mdickinson]

Merging this after re-review.

It's not ideal: one inelegance is that in the case of accessing the shadow trait for the first time (without first accessing the primary trait), mapped_value is called twice with the same input. But I think it's good enough for the release.