Teach GrampsType.set() to work with a dict

[stevenyoungs]

Teach GrampsType.set() how to work when value is of dict type
This fixes a bug introduced by PR #1786

To reproduce:

1. create a new, empty, tree
2. switch to event view
3. create a new event, changing the event type to Death
4. observe that the event type is shown as Birth in the event view

[stevenyoungs]

@dsblank I'd appreciate your review of this
I believe the problem stems from the most recent change to eventmodel.py
return str(EventType(data[COLUMN_TYPE]))
was changed to
return str(EventType(data["type"]))

data[COLUMN_TYPE] is a tuple whilst data["type"] is a dict.
GrampsType.set() does not know how to set for a dict. I'm assuming there will be other instances of this problem in the code base - but I've not verified this statement!

[stevenyoungs]

The same problem exists with notes and repositories views.

[dsblank]

@stevenyoungs good catch! Yes, you are correct... another place where the array/blob representation had leaked into other parts of the code.

What would be better, though, would be to not create an object if it isn't necessary, and just get the results needed. That might be something like:

def get_event_type_from_raw_data(data):
    return what_ever_is_needed

[dsblank]

Can't we just from_dict(data["type"]) ?

[stevenyoungs]

@stevenyoungs good catch! Yes, you are correct... another place where the array/blob representation had leaked into other parts of the code.

What would be better, though, would be to not create an object if it isn't necessary, and just get the results needed. That might be something like:

def get_event_type_from_raw_data(data):
    return what_ever_is_needed

How about this, which allows us to avoid building the object, and also hides the detail inside a static method in EventType

    @staticmethod
    def get_str(value):
        if value["value"] == EventType._CUSTOM:
            return value["string"]
        else:
            return EventType._I2SMAP.get(value["value"], EventType.UNKNOWN)

and then it becomes

    def column_type(self, data):
        return EventType.get_str(data["type"])

Thoughts?
Is it worth leaving the existing change in grampstype in case other (external) code has the same problem?

[dsblank]

I'm going to do some timing tests between turning just the data["type"] into an instance, versus your static method. If they are not that significantly different, I think we'd just create an object, rather than having to have yet another method to access the data.

[dsblank]

Timing tests comparing converting to an object (using an updated #1824) vs static method (above) with Example tree:

Setup:

from gramps.gen.db.utils import open_database
from gramps.gen.lib.eventtype import EventType
db = open_database("Example")

Turning into an object (uses old, standard Gramps code; no changes to code base once #1824 is merged):

In [16]: %%timeit
    ...: for handle, event in db._iter_raw_event_data():
    ...:     x = str(event.type)
    ...: 
65.8 ms ± 1.02 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

Using the static method (requires an import):

In [20]: %%timeit
    ...: for handle, event in db._iter_raw_event_data():
    ...:     x = EventType.get_str(event.type)
    ...: 
    ...: 
32.2 ms ± 573 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

So, the static method is twice as fast (saves 30 ms over 3,432 events), but at the cost of having to know about a special static method and requiring an import.

Personally, I don't think it is worth it, but I support you in whatever direction you want to go.

[dsblank]

@stevenyoungs, what do you want to do to fix the bugs that you found?

[stevenyoungs]

@dsblank I'll hopefully get some time over the coming days to come back to this.
My current thinking was to provide both the current as well as the staticmethod approach.
For a large tree, I'm assuming that the standard views want the type frequently enough to justify providing both approaches. Let me know if you have any concerns about this.

[dsblank]

For a large tree, I'm assuming that the standard views want the type frequently enough to justify providing both approaches.

That sounds fine to me! It is there if one wants to use it. #1824 has been merged, so either of the code examples above will work.

[stevenyoungs]

@dsblank I revised it to an @classmethod and implemented in GrampsType. This way it works for all types, not just EventType
Relevant model classes updated and their column_type methods made consistent
I'd appreciate your review.

[dsblank]

I left some questions/comments, but it is better than before, and also fixes a bug or two. Thanks!

[stevenyoungs]

This is a bug fix for #1786 (which was an enhancement). Can someone add the bug label please?

[dsblank]

Ready to merge, IMO.

[Nick-Hall]

Only one real issue.

[stevenyoungs]

@Nick-Hall this is ready for re-review. Thanks.