Neriderc / GVExport

Repository for GVExport module for Webtrees
GNU General Public License v2.0
15 stars 6 forks source link

Option "Mark not blood-related people with different colour" #36

Closed hartenthaler closed 2 years ago

hartenthaler commented 2 years ago

What is the meaning of this if you add more than one root persons? As far as I have seen the meaning is "if a person is blood-related to any of the root persons, then it has another color as persons that are not blood-related to any of the root persons. But is this of value for the user? What happens if a root person is blood-related to another root person? What happens if a root person is related, but not blood-related, to another root person?

When using persons and families from the clippings cart instead of the internal function then this option makes no sense anymore, because there are no root persons in the clippings cart.

hartenthaler commented 2 years ago

And this function is not implemented correctly, because adopted or foster children are marked as blood-related, what is wrong.

Neriderc commented 2 years ago

I use this option and find it of value. For example I might choose one of my kids as the starting person. Then I select to include all siblings, cousins, and spouses, going back for say 3 or 4 generations. This will print out a big tree of everyone they are related to, plus spouses. Having the spouses in a different colour means it's easy to see blood relatives vs people who are related only be marriage. It also makes it easy to spot why the tree hasn't continued to grow from that point despite there being data for relatives of those non-blood related individuals.

With multiple starting people, if a person is a blood relative of any of the starting individuals then they are considered a blood relative, otherwise they are not a blood relative.

For the clippings cart, perhaps this could be ignored?

hartenthaler commented 2 years ago

I see the value if there is exactly one root/starting person. This is helpful.

If there is more than one starting person, then there should be a color for each starting person and the other persons which are blood-related to this starting person. And there should be another color for another starting person and their blood-related persons. If they have all the same special color it is meaningless to me in this situation.

If the meaning is "blood-related" then foster children and adopted children should not be marked using the "blood-related" color.

For the clippings cart I can set "related" for all persons to false or to true. Maybe false is better.

Neriderc commented 2 years ago

I think for multiple starting people, having different colours for each would get complicated. What do you do when someone is related to 2 or 3 or 4 of the starting people? I don't have a problem with the current implementation, which is that if you are related to anyone in the starting list then you get the standard pink/blue, otherwise you get the faded pink/blue for non-relatives.

I agree adopted/foster children are not blood related and so this should should show as such (but it would have to take into account when someone is adopted but is blood related, e.g. if someone adopts a niece or nephew or grandchild).

For the clippings cart: I see the standard pink/blue as the default, and only if you choose you want a different colour for non-blood relatives then there are new colours introduced. So I would think with the clippings cart all should be the default pink/blue. I haven't checked the code but I presume this would be setting "related" to "true" for all of them.

hartenthaler commented 2 years ago

Yes, to check if an adopted child is maybe related via another path is necessary.

It would be nice to have more colors for the links between persons: black for "birth", blue for "adopted", green for "foster", and grey for anything else (sealing, rada, ...). Hmmm... there is "adopted by both parents/by father/by mother"; I think this is too complicated and should be neglected.

For the clippings cart: your logic is perfect. It was already set to "true" for all people.

Neriderc commented 2 years ago

Just thinking, in regards to this and #32 as well, this option does nothing when there are items in the clippings cart. It would be the only option outside of the "Diagram preferences" that is disabled by the clippings cart, but if we left it in the "Diagram preferences" then it would be the only option that affects appearance rather than who is included.

Moving it to Appearance makes it more difficult to explain what parts are disabled and why.

I guess an option could be for it to move to Appearance, but have it's own little message about why it's disabled?

hartenthaler commented 2 years ago

I see the problem. Good point.

I would prefer to keep it in the "Diagram preferences" to make the logic clear. My suggestion: rename "Diagram preferences" to "People to be included". Then this option "mark not blood-related ..." fits better.

hartenthaler commented 2 years ago

"People to be included" could have two alternative sections: "Select People" and "Use clippings cart".

Neriderc commented 2 years ago

Yes I've considered that we should give the option to override the clippings cart, if you don't want to clear it but want to generate a diagram without it. I think that can be an enhancement, we'll just get the basics working then can add that later.

How does this look?

image

hartenthaler commented 2 years ago

Perfect for the moment. Add a selection box "use/not use clippings cart" could be an enhancement in the next step.

Neriderc commented 2 years ago

Great! I think it's fully working, but will test a bit more tomorrow. If it's all working ok, I will merge all the outstanding pull requests.

I may also revise the wording of the message. It was the first thing I thought of so will see if I can think of something better, or shorter.

hartenthaler commented 2 years ago

Thank you!

I'm doing some more testing on the new import from the clippings cart, too. Introducing the dummy "families" and "individuals" is not a perfect programming solution. Can be done better.

Neriderc commented 2 years ago

If you have further changes, you should be able to just update your ClippingsCart branch on your fork and it should add the commits to the push request. I'll test what's there tomorrow, feel free to push changes if you want. After it's merged you'll need to make a new pull request if you want to make changes, but that's no problem. Github should walk you through it, letting you know which you need to do.

Neriderc commented 2 years ago

With all the conversation here I'm a bit lost on what we ended up on for this. I'd like to work out what actions we have here that aren't covered in another issue.

Is it just: Adopted/foster children should not be the colour of blood relations (when this option is enabled)

We disabled the blood related option for clippings cart, and the link colour is covered in #46, so I'm not sure there's anything else left in this issue other than the bug with adopted children?

hartenthaler commented 2 years ago

Yes, it is only about the color of the box for adopted/foster/sealed/rada children. It is only relevant if there are no INDI in the clippings cart and if the option "mark non-blood-related ..." is checked. At the moment adopted children are marked blood-related, but they should not. But even if they are not blood-related to their adoptive parents, they may be blood-related via their biological parents to any of the source/root individuals. I don't know how to show this: maybe then blood-related is more important than not-blood-related.

Neriderc commented 2 years ago

Ok so related to this - what is the correct way to mark someone in webtrees as adopted?

It seems there is code in this module that specifically marks adopted children as not related - it might be broken but I'm simply not sure how to correctly add an adopted child in webtrees (in particular in 2.1.6). The way I thought it worked was parents are the biological ones, and then you have an adoption event that links to a family record of the adopted parents. As far as I'm aware this is the complete setup, but doing this doesn't seem to show them at all in GVExport. Am I doing it wrong or is it simply not working?

You mentioned adopted children showing as blood related so I'm interested in knowing how you've set this up since my test cases don't show at all.

hartenthaler commented 2 years ago

I have in the INDI record of an adopted person:

1 FAMC @F789@
2 PEDI adopted

1 ADOP
2 FAMC @F789@
3 ADOP BOTH

F789 is the family of the adopting parents.

For another person I have (without an event):

1 FAMC @F123@
2 PEDI FOSTER

If you have only the adopting event this is not enough, you have to link the adoptee to the adopting family. Go to the adoptee. Open the family tab. Select the option "Link this individual to an existing family as a child". Select the adopting family and select at "Relationship to parents" the value "Adopted".

Neriderc commented 2 years ago

Great, thanks! I have it working now, and see the issue, so when I get time I'll try to solve it.

Neriderc commented 2 years ago

Here's a problem. With the situation of an adopted child that is also related (as the example from before with a family adopting a nephew/niece), we add the child in as adopted and they show as non-related. Normally the tree grows, finds the link via sibling, and updates to show they are related after all.

But if we are showing just the family with the adopted child and not the rest of the family, that link is never checked, and the adopted child is shown as non-related.

I don't think it's practical to scan the entire tree looking for links every time we want to generate a tree (as this would make it take unnecessarily long), but how else can we solve this problem?

Do we just accept that there will always be situations where it may be displayed incorrectly?

hartenthaler commented 2 years ago

I think your analysis is correct.

To make it correct, checking for any person (who has to be shown) if this person is blood-related to any of the starting persons is necessary, checking their whole ancestor tree. To make it efficient I suggest building up an array with all blood-related persons of the starting persons once and then check for all persons who have to be shown if they are in this array.

Otherwise, we have to accept that the color indicator "Blood-related" is not correct in all cases. But what would be the value of such an indicator? In simple cases, the indicator is not necessary because we all know that a person is blood-related to the parents and the biological children. And we know that a husband is normally not blood-related to his wife or that an adopted child is in most cases not blood-related to the adopting parents. The color indicator is helpful because it shows if there is an abnormal situation. But then it is necessary that the indicator is correct in all cases.

But even, if we check the whole tree of ancestors and their descendants we might be wrong. Because there are maybe cuckoo children or because the tree is not complete (all people are related to all other people if you go back many thousands of years). I would accept this uncertainty.

Neriderc commented 2 years ago

To make it correct, checking for any person (who has to be shown) if this person is blood-related to any of the starting persons is necessary, checking their whole ancestor tree. To make it efficient I suggest building up an array with all blood-related persons of the starting persons once and then check for all persons who have to be shown if they are in this array.

I'm not sure how practical this is. This involves scanning the entire tree (ignoring any limits set by the settings). While possible, I have thousands of records in my main tree. Some people have tens of thousands. If you want to display a couple of generations, it would be impractical to scan tens of thousands of records to generate this array for checking against. Instead of 2 seconds you'd be waiting minutes to generate the simple tree. You could of course only do this scanning when the option for non-relatives in a different colour is selected, and when an adopted record is found, but I'm still not convinced of it's practicality.

Otherwise, we have to accept that the color indicator "Blood-related" is not correct in all cases. But what would be the value of such an indicator? In simple cases, the indicator is not necessary because we all know that a person is blood-related to the parents and the biological children. And we know that a husband is normally not blood-related to his wife or that an adopted child is in most cases not blood-related to the adopting parents. The color indicator is helpful because it shows if there is an abnormal situation. But then it is necessary that the indicator is correct in all cases.

But even, if we check the whole tree of ancestors and their descendants we might be wrong. Because there are maybe cuckoo children or because the tree is not complete (all people are related to all other people if you go back many thousands of years). I would accept this uncertainty.

You're right in that we don't know for sure if an adopted person is not related, as they could be related further back in the tree. There is always an element of uncertainty. But if we know, based on the available data, that someone is related, then it should be shown as such. Otherwise, show as unrelated, and this makes it clear at a glance that there is something abnormal happening there. The user can investigate further if they wish.

I'm thinking a simpler way of handling this is to simply tell the user about the uncertainty. I have been thinking we need a way to present information to the user. Personally I would like to be told how many records have been added to the tree. I think a simple "toast" message would be a good way to do this. A small message that pops up in the corner saying something like "Diagram completed: XX individuals and XX families displayed in the tree".

This could be extended with warning messages. e.g. "Warning: Adopted records found and displayed as non-related as link not found in selected records. Relationship may exist outside of displayed records".

This would solve the problem and put the onus on the user to confirm if it's correct or not.

hartenthaler commented 2 years ago

I'm not sure how practical this is. This involves scanning the entire tree (ignoring any limits set by the settings). While possible, I have thousands of records in my main tree.

It is not necessary to scan the entire tree. Only all biological ancestors of the starting persons and their biological descendants. Even if you have 15 generations I would assume that it takes only one second to build up this array because the data structure is simple (add recursive all parents and all of their descendants). For comparing it: if you are using the Vesta modules, you can show in the individual tab for the shown person the relationship to you. This is a much more complex function. I have this function switched on and it shows even in very complex situations a result in less than five seconds.

Personally I would like to be told how many records have been added to the tree. I think a simple "toast" message would be a good way to do this.

I had the same idea already some time ago. I would be happy to get this information be shown.

Neriderc commented 2 years ago

It is not necessary to scan the entire tree. Only all biological ancestors of the starting persons and their biological descendants. Even if you have 15 generations I would assume that it takes only one second to build up this array because the data structure is simple (add recursive all parents and all of their descendants). For comparing it: if you are using the Vesta modules, you can show in the individual tab for the shown person the relationship to you. This is a much more complex function. I have this function switched on and it shows even in very complex situations a result in less than five seconds.

Depending on the tree and starting persons selected, you could be scanning nearly the entire tree. And with multiple starting people, this could be more likely. In theory it shouldn't be super complicated, because we already build an array like this. Hopefully the existing function can be reused. But I do think this should be a separate piece of work. It has the potential to be quite substantial (depending on how fit for purpose the existing function is) and I think it's not unreasonable for an adopted person to be marked as unrelated when they are distantly related by a connection not shown in the tree, so long as the user is aware. At the very least it's better than what we have now, which is assuming adopted children are related.

I had the same idea already some time ago. I would be happy to get this information be shown.

Great!

I will merge in what I have for fixing this issue, then create two new issues to be worked on in future.

  1. Scan entire tree for blood relatives in order to confirm if adopted child is blood relative
  2. Create "toast" message system, with the initial messages to be one that tells you how many records were added, and another to warn about adopted children that will be shown until the first issue is completed,
hartenthaler commented 2 years ago

very good!