Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception when finding relationship to home person #1803

Open
wants to merge 1 commit into
base: maintenance/gramps52
Choose a base branch
from

Conversation

hgohel
Copy link
Member

@hgohel hgohel commented Nov 30, 2024

Fixes #13495

Proposed bug fix to catch the unhandled exception by testing for None. However, it's not a guaranteed fix because no data was provided to test the bug.

Question remains what is the family tree that leads to the exception? Is it a valid case? Should it be handled in a way that isn't being done now? If it isn't valid, is it being captured by check and repair database?

Fixes #13495

Proposed bug fix to catch the reported exception by testing for None. However, it's not a guaranteed fix because no data was provided to test the bug.

Question remains what is the family tree that leads to that situation? Is it a valid case? Should it be handled in a certain way that isn't being done now? If it isn't valid, is it being captured by check and repair database?
@emyoulation
Copy link
Contributor

Perhaps there should be a standard function for testing if the minimum data exists for a feature or plugin?

The typical requirements are:

  • a tree loaded
  • a record of a particular Primary type (person, place, event, etc.) exists
  • an Active Record of a particular type exists
  • a Home Person exists
  • attributes (Date or Place for an Event, coordinates for a Place, citation) for the Priimary

If the requirement doesn't exist the feature is dimmed and has an override tooltip.

@hgohel
Copy link
Member Author

hgohel commented Nov 30, 2024

Perhaps there should be a standard function for testing if the minimum data exists for a feature or plugin?
...
If the requirement doesn't exist the feature is dimmed and has an override tooltip.

Possibly. In this case all these checks may have passed, but the code ran into some data that wasn't handled properly and thus the exception. If test data was available, it could have led to further changes in the find relationship code, or perhaps check and repair database (if appropriate).

Disabling features or adding tooltips is a reasonable idea, but it could be expensive as noted in a recent thread in Discourse.

@emyoulation
Copy link
Contributor

I've added these checks (with feedback in the main window instead of tooltips) to the the experimental What's Next? gramplet. (And then added the necessary signal monitoring as another part of the learning curve).

But then found the Pedigree gramplet was pushing extraneous warnings to the log when no tree was loaded or there was no Active Person. I'm not looking forward to adding such sanity checks to the other Gramplets.

Serge did 2 experiments with Gramplet "Freeze" options to keep them from eating resources in the background. Maybe they would be compatible with a sanity check for Gramplets.

0010850: Freeze contextual updating to Gramplet content?
https://gramps-project.org/bugs/view.php?id=10850

@hgohel
Copy link
Member Author

hgohel commented Nov 30, 2024

@emyoulation Good to see some work in this area. Without data to pin-point the root cause however, it's not clear to me what action can be taken here. Maybe the discussion should continue on Discourse rather than this PR.

@emyoulation
Copy link
Contributor

I open too many "Idea" topics.

However, the thought occurs that this could be a 5.3 or 5.4 expanded functionaly in the 5.2 addition of 3 'Requires' (_mod module, _gi GObject introspection, _exe executables ) attribute to plugin registration.
#1451
Line266-Line275 https://github.com/gramps-project/gramps/pull/1451/files#diff-b7237ade5cc2361c0cc8fe3579b783732a52c21547375313ae1ef1115b8db1c5R266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants