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

GEPS 045 - a second attempt at enhanced places #809

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

prculley
Copy link
Contributor

@prculley prculley commented Apr 12, 2019

An upload for GEPS 045: Place Model Enhancements in case anyone wants to see if I'm going in the right direction.

At this point I think that all the code should work, so any remaining issues are bugs. While it is possible to upgrade a tree with the current db, (and a backup is automatically made of the db first) please use care on your special trees. Import/export of XML works, as well as the other importers and exporters. All reports should work. The gramps-project/addons-source#214 contains the addons also updated to work with this PR, since they have not been built/uploaded, they will have to be installed manually.

A few notes;

  • Place names and place types are actually unified lists, not a primary and alternate list like previously. You can still edit the top item in these lists in the top of the dialogs, changes appear in the displaytabs. Users who don't mess with multiple names and types should have no trouble.

  • I made the new editors as much like the old ones as possible, so they should be familiar to previous users.

  • The place types list is still the same as always, just the previous Gramps types. Users of GetGOV will find additional custom types appearing in their data and on the place type combos.

  • GOV types are added as numbered custom types. Custom types are either 'numbered' or fully custom; the numbered types are assigned in a range starting from -1 and counting down, fully custom types while internally numbered (in a different negative range) don't preserve the number for XML/GEDCOM transfer, while numbered types do.

  • To deal with the many place types, I am adding the concept of place groups. These groups are used to make the place types combo menu a bit easier, and to allow selection and display of hierarchies and groups for titling. For example, Gramps originally had the concept of Country, County, City type etc. Since there are now multiple place types which could be described as countries, we use the Countries group as a group designation for Countries. The groups are as follows:

    • Countries - for all nation states and confederations. e.g. Kingdom, Republic.
    • Regions - for all administrative and geographic regions. e.g. State, County.
    • Populated Place - for all types of settlement. e.g. City, Town, Village, Hamlet.
    • Unpopulated Place - e.g. Cemetery, Battlefield.
    • Building - e.g. House, Church, Hospital.
      The old fields COUNTRY and CITY can then map to Country and Populated Place.
      The old fields STATE and COUNTY can be determined by the actual position of a Region within the place hierarchy. STATE will be a top level Region and COUNTY a second level Region. LOCALITY is simply a Region below the Populated Place level.
  • The Groups are stored with the place types as an integer bit field. This makes identifying a place type as belonging to a group a simple bitwise 'and' operation.

  • I've added a place types configuration dialog to allow adding, removing, renaming, and changing groups possible for each place type. It is also possible to add/remove Groups. Preferences/PlaceTypes.

  • The place enclosure dialog now includes a place hierarchy. If a user adds a hierarchy (by typing a custom name in the combo), it also adds a place type group with the hierarchy name, which can be assigned to custom place types. For fun I tested with an astronomical hierarchy with planet, system and galaxy place types.

  • postal codes and some other bits of data are now supported as Place Attributes. If dates or types are attached, that information is added as text to the attribute data. Postal codes from previous XML trees are converted and stored as place attributes (not dated).

Screenshots of the new and modified dialogs are https://gramps-project.org/wiki/index.php/GEPS_045:_Place_Model_Enhancements_-_Place_Changes_Screenshots.
Comments welcomed.

@Nick-Hall
Copy link
Member

Thanks for this new PR.

My first reaction is that the place type groups still appear to be hard-coded to the GOV groups. The user should be able to define their own groups or decide not to use groups at all. We don't want to adopt the GOV groups for core Gramps, but allow users to define the GOV structure if they decide to use the GOV database.

@prculley
Copy link
Contributor Author

@Nick-Hall I understand. However, I would still like to keep at least the Admin hierarchy and the ADMx groups as part of our new standard. The others could easily be added as needed by GOV data or the user. The reason is that many of our reports and map views still use the country/state/city concept for their data presentation. I've changed (some of) their code to use the groups so that they make sense even when other place types are used. If I revert to just using place types in these reports/views, then they don't work well when GOV data is used and any of several types could be considered to be city class (or country class etc.).

@Nick-Hall
Copy link
Member

@prculley No. We shouldn't hard-code the GOV groups into core Gramps.

Users wishing to use the GOV database should be able to create the GOV groupings.

However, we should start with no groups or perhaps two or three at the most, For example, "Administrative" and "Places" would probably be acceptable, but not all of the GOV groupings.

@bmatherly
Copy link
Contributor

No. We shouldn't hard-code the GOV groups into core Gramps.

I think I tend to agree with this. On the other hand, we don't want to make it impossible or difficult for those who do want to utilize GOV groups. Maybe GOV could be a classification of custom types? So it isn't a 1st class citizen in Gramps, but it maps in/out very easily? Just thinking out loud.

"""
Place Type class.

This class is for keeping information about place typess.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "typess"


This class is for keeping information about place typess.
This class is similar to GrampsType based classes, but has other features.
It supports 3 ranges of types;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading this, it seems like we are giving a lot of responsibility to a single value. Would it improve the clarity of the code if we added another member to keep track of what "kind" of type it represents?

@ennoborg
Copy link
Contributor

ennoborg commented Apr 13, 2019

When I run this in Dutch, I see 2 instances of Dorp (Village), 1 Dorpje (little village, not an official type), 1 Dorp of Stad (Village or City), and 1 Plaats, which is the common name for an official PPL. I also see Deelstaat, Provincie, and Deelstaat/Provincie. And these are just the standard types of Gramps 4.2. See below:

Dutch place types

For me, this means making a messy product even worse. Why can't we clean things first?

@Nick-Hall
Copy link
Member

For me, this means making a messy product even worse.

I agree.

The approach I outlined as a result of the user consultation listed the following requirements for place type and groups:

  • We should keep the standard place types, but keep the list short.
  • The user should be able to hide/remove any standard place type.
  • The user should be able to define a custom place type.
  • Custom place types should be able to store a reference to an external data source (e.g. GOV).
  • The user should be able to put any place type into a group.
  • The list of groups should be user-definable.
  • The use of groups should not be mandatory.

and the following requirements for rules:

  • A place format should allow the user to optionally define a list of formatting rules.
  • Each rule should refer to either a place type or group of place types.
  • A rule should optionally be restricted to a specified country (or other top-level place type).
  • A rule should optionally be restricted to a specified level in the place hierarchy.
  • A rule should define an action, such as hiding a matching node in the place hierarchy.

In addition I specified a couple of dialogs for a new place format editor.

The goal was to be flexible, allowing a simple structure for normal users and more complex possibilities for those that need them. Note that in particular I specified that users should be able to define both the groups and the types they wanted to see within them.

Whilst this PR is a step in the right direction, I don't think that it is acceptable in its current state.

@Nick-Hall
Copy link
Member

I should add that I have no objections to the time-dependent place types, abbreviations, hierarchy type, place attributes, place events or new citations. Perhaps we should split these out from this PR so that they can be merged?

@bmatherly
Copy link
Contributor

bmatherly commented Apr 14, 2019

I should add that I have no objections to the time-dependent place types, abbreviations, hierarchy type, place attributes, place events or new citations. Perhaps we should split these out from this PR so that they can be merged?

I love that idea. I think the place type values are the only things we are still working out.

@prculley
Copy link
Contributor Author

* We should keep the standard place types, but keep the list short.

ok, and done if you comment out the types added for testing.

* The user should be able to hide/remove any standard place type.

done

* The user should be able to define a custom place type.

done

* Custom place types should be able to store a reference to an external data source (e.g. GOV).

The proposed config dialog shows the type number for GOV types; if we want a more general string a fairly simple addition can make this happen.

* The user should be able to put any place type into a group.

done

* The list of groups should be user-definable.

Partly done, in that user defined custom hierarchies show up as new groups. And I have a method call to add new groups. But we may want more explicit add/remove buttons as well...
As I mentioned above we can easily remove many of the predefined groups in this PR, they could be added again as GOV types started using them, either automatically via GetGOVv2 or manually, but...

* The use of groups should not be mandatory.

This is where I'm still having problems. Our current code base (prior to this PR) contains a number of occurrences of hard coded Place Types, mostly to COUNTRY, STATE, CITY and sometimes COUNTY or one or two others.

My original thought was to replace these with Administrative level groups, which the GOV folks had defined and which is also used by the GeoNames. By making our original Gramps types members of the appropriate groups, the code that had the hard coded place types could be fairly easily converted to use hard coded groups. I've implemented this concept for the Geography View as a test and it seemed to work ok. Users that never used GOV data would not have noticed the difference, and GOV users would get pretty good compatibility.

But if we cannot have even a few hard coded groups, then we have a problem for the views/reports that used to use the hard coded place types. The Geography view for instance just categorizes everything as 'Other' for the right-click 'link place/Place selection in a region' when GOV data is stored in the db., or uses the 'UNKNOWN' value for color.

I suppose that I could add work items to do more major changes to the reports/view that had hard coded place types, perhaps changing them to use the place.displayer code instead.

I do think leaving this the way it is now is at best inelegant, and really makes the work seem unfinished. I'd like to do it as right as possible.

I'd really like to come to a final agreement on how we are going to handle this. I also think that breaking this into two PRs would increase the work quite a bit, as there is a fair amount of code overlap.

@Nick-Hall
Copy link
Member

Nick-Hall commented Apr 14, 2019

Great work! It seems you have made progress.

This is where I'm still having problems. Our current code base (prior to this PR) contains a number of occurrences of hard coded Place Types, mostly to COUNTRY, STATE, CITY and sometimes COUNTY or one or two others.

I don't really have a problem with a nation state group or a populated place group. These are natural concepts to understand. The types Hamlet, Village, Town and City are already hard-coded together in the place displayer, which is horrible - I'd like to get rid of that.

It's getting late now. Let me think about this further overnight.

@Nick-Hall
Copy link
Member

After further thought, I think we need a few broad place type groups that are easy for users to understand.

Country - for all nation states and confederations. e.g. Kingdom, Republic.
Region - for all administrative and geographic regions. e.g. State, County.
Populated Place - for all types of settlement. e.g. City, Town, Village, Hamlet.
Unpopulated Place - e.g. Cemetery, Battlefield.
Building - e.g. House, Church, Hospital.

The old fields COUNTRY and CITY can then map to Country and Populated Place.

The old fields STATE and COUNTY can be determined by the actual position of a Region within the place hierarchy. STATE will be a top level Region and COUNTY a second level Region. LOCALITY is simply a Region below the Populated Place level.

Of course we should check whether it is still sensible to use these old field mappings, but that doesn't necessarily have to be part of this PR.

@prculley
Copy link
Contributor Author

I've updated the code to reduce the groups to those you asked for. This did entail a few other changes. I've also added a group add/remove right-click popup menu to the 'Edit/Preferences/Place types' dialog to allow the user to configure his own place groups. This time I commented out the GOV types/Groups so testing should show how a clean version of Gramps should look.

For testing purposes, I have also uploaded a modified version of GetGOV (GetGOV_GEPS045) which takes advantage of the new features. This version doesn't add any groups, although I'm thinking of adding an option that lets it add groups if places with types belonging to those groups are imported (If a user imports a diocese for example, the 'Religious' group could optionally be added as well).

At them moment, I think you should be able to import older XML trees, and import/export the current trees. I think all the views work, as well as the various place related dialogs. I've also gotten a few of the place related filters tested and updated.

If you download and test, I recommend that you trash any previous dbs and XML exports made from the GEPS045 PRs as they may not be compatible.

There is still a lot of work left to do, even to ensure that most everything still works, much less to get it to deal with the enhanced places. So I am going to leave this as "work in progress" while I deal with that. Otherwise master branch is likely to have a lot of bugs that could prevent others from working on it effectively.

Unfortunately the time I have for this has has dropped a lot lately, so things are going slower... But I still intend to get it all done.

@prculley
Copy link
Contributor Author

prculley commented Apr 23, 2019

and the following requirements for rules:

* A place format should allow the user to optionally define a list of formatting rules.

* Each rule should refer to either a place type or group of place types.

* A rule should optionally be restricted to a specified country (or other top-level place type).

* A rule should optionally be restricted to a specified level in the place hierarchy.

* A rule should define an action, such as hiding a matching node in the place hierarchy.

In addition I specified a couple of dialogs for a new place format editor.

@Nick_H did you post your glade files anywhere so I could start with those?
A couple of comments, as I start thinking about implementing the displayer/format editor;

  1. I think some folks want to be able to change the number/street order depending on place, so I'm inclined to make this some kind of rule.
  2. I foresee the ability to define rules that are in conflict with each other; so I'm thinking that we need to have some sort of priority on the rules, perhaps most general rule at top, to most specific rule at bottom, where lower rules override upper ones.
  3. I see you are putting the hierarchy choice at the top level, so that seems to imply you cannot display more than one hierarchy at a time. Possibly an issue (for some reports at least)?
  4. I'm leaning toward a default (not explicit) rule of 'show all, no abbreviation', so only rules that override this default have to be defined.
  5. Since the place custom types/groups and place-format 'where' value can all be changed by the user and are traditionally db specific I think the place formats end up as db specific as well. Not an issue for any single db, but may be annoying to users who have multiple dbs and have to set this up for each one.
  6. I'm thinking that to ease users concerns, we should have a few default formats;
  • Full: everything in the Admin hierarchy
  • Views: everything except Country and region reduced to abbreviations, when available, maybe also smallest of Countries (so we don't see EU in front of every European country).
  • Graphs: don't show street/number, only show largest of Places, and largest of Regions, and show Country/Region as abbreviations.

@prculley
Copy link
Contributor Author

I just added a place format editor and associated displayer. I think it works, but will take some more debugging to be sure. Unfortunately I will be very short on time for the next week+ so it may be a while before more work is completed. But any comments are appreciated.
PlaceTypeConfig
PlaceFormat_1

@ennoborg
Copy link
Contributor

ennoborg commented May 1, 2019

Closing the Place Types dialog gives me an error:

159982: ERROR: grampsapp.py: line 150: Unhandled exception
Traceback (most recent call last):
File "/home/enno/gramps-culley/gramps/gui/configure.py", line 214, in done
self.__on_close()
File "/home/enno/gramps-culley/gramps/gui/configure.py", line 541, in _close
self.dbstate.db.save_place_types() # save place types data.
AttributeError: 'DbBsddb' object has no attribute 'save_place_types'

I also like to see the English types, because now when I see 'Dorp' multiple times, I can't see which is which, should I wish to record the proper type for a country where the difference IS relevant.

@ennoborg
Copy link
Contributor

ennoborg commented May 1, 2019

I get another error when I try to open the Place Format Editor:

52486: ERROR: grampsapp.py: line 150: Unhandled exception
Traceback (most recent call last):
File "/home/enno/gramps-culley/gramps/gui/editors/editplaceformat.py", line 99, in __format_changed
self.__load_format(fmt)
File "/home/enno/gramps-culley/gramps/gui/editors/editplaceformat.py", line 175, in __load_format
custom_hier_types = sorted(self.dbstate.db.get_placehier_types(),
AttributeError: 'DbBsddb' object has no attribute 'get_placehier_types'

Both errors don't appear when I use an SQLite database.

@romjerome
Copy link
Contributor

romjerome commented May 2, 2019

I also like to see the English types, because now when I see 'Dorp' multiple times,

Would love to see a 'grouping' too.
French translation also needs to deal with types and also cultural issues between France, Canada, Belgium, Switzerland, etc ... like English locales.

eg, inherited from gedcom types or American model :

# comté (Canada)
msgid "County"
msgstr "Comté (Départ.)"

# province (Canada, Belgique)
msgid "State"
msgstr "Province (Région)"
# province (Canada, Belgique)
msgid "Province"
msgstr "Province"

msgid "Department"
msgstr "Département"
msgid "District"
msgstr "District (Arrondissement)"

msgid "Borough"
msgstr "Borough (Arrondissement)"
# Canada
msgid "State/County"
msgstr "Province/Comté "

# Canada
msgid "_State/County:"
msgstr "_Province/Comté :

@prculley
Copy link
Contributor Author

prculley commented May 2, 2019

I don't have support for bsddb or proxydb or ... in place yet, lots of work yet to do. I should have copied the comments to that effect from the other GEPS045 start.

@Nick-Hall
Copy link
Member

@prculley Sorry, I have only just seen the latest updates. For some reason I am not getting the usual email notifications. Will make some comments soon. It certainly looks like you are making progress, but not quite there yet.

@Nick-Hall
Copy link
Member

@prculley This is looking better, but I don't think the design of the place types and place formatting is correct yet. Both @ennoborg and @romjerome make valid points that need careful consideration.

Since the code freeze is only two days away, this PR can't be included in v5.1 in it's entirety. It probably isn't even sensible to split the PR at this point. We don't really want a rushed job.

@Nick-Hall Nick-Hall modified the milestones: v5.1, v5.2 May 14, 2019
@prculley
Copy link
Contributor Author

@ennoborg @romjerome Gentelment, I'm not too clear on what you mean in your comments.

I also like to see the English types, because now when I see 'Dorp' multiple times, I can't see which is which, should I wish to record the proper type for a country where the difference IS relevant.

Is this situation because you have utilized GetGOV to import and manage places, along which will come GetGOV's types, some of which have poor translations and conflict with the Gramps types? Or something else?

I'm not clear what 'grouping' means. And I'm also unclear what you would like to do with the "French translation also needs to deal with types and also cultural issues between France, Canada, Belgium, Switzerland, etc ... like English locales."
My understanding of Nicks instructions is that I should not be changing Gramps standard types, in particular to do anything with the GetGOV types. The 'grouping' of similar types like 'state, territory' etc. was rejected by Nick, at the moment, everything between Country and place (city equivalent?) is in a single group called 'region'. I'd appreciate some more specific suggestions, which I am happy to code up, but I think we need a better overall agreement and understanding of the issues and solutions before I make more changes. I'd like to avoid having to redo this too many more times.

@emyoulation
Copy link
Contributor

Patrice Legoux recently linked to a podcast by an experienced genealogy software user's first encounter with Gramps creating a small Tree.

The commentary was in French but it was obvious that the part that caused utter confusion was the creation of a Place hierarchy. Specifically, she was OK until adding an enclosing Place. But the spawned New Place window overlaid the window she had just filled in... and the new Enclosing Place dialog gave the APPEARANCE as though the parent window had been blanked, wiping out her work.

There is room above the Name & Type row in the New Place dialog. How about putting in a parentage reminder of the Spawning/Parent window. (Which might be a little tricky since those records won't have been committed to the database.) This example is in plain text, but italics might look less like a title and more like an annotation.
image

It seems like there would be only a few spawning opportunities for the New Place dialog, requiring a different form of parentage reminder:

  1. a New Place (from the Add menu or the top of the hierarchy in the Place category)
    New place
  2. a New enclosing place (from a Edit Event spawned New Place dialog)
    New place enclosing <place>; <Event Type> place of <Person name>
  3. a New enclosed place (from the Place category lists)
    New place enclosed by <place title>
  4. a New Place (from the Edit Event dialog)
    New place for <Event Type> of <Person name>

@emyoulation
Copy link
Contributor

emyoulation commented Jan 21, 2023

The New Place/Edit Place dialogs would be less obfuscated with cleaner titles around the Latitude and Longitude fields and the Parser field.

Could you simply ELIMINATE the overly verbose labels "Either use the two fields below to enter coordinates (latitude and longitude),"

and replace "or use copy/paste from your favorite map provider (format: latitude, longitude) in the following field:"

Creating a pop-up menu selector labeled something liek "Quick Entry:" or "Place phrase parser:" that might encourage new directions of development.

One parser menu item could be 'coordinates (format: latitude, longitude)' that passes through the current field data entry validator.
But a second parser menu item could repurpose place title handlers ('data entry' gramplet, 'extract place data from a place title' family tree processing tool) that will parse a fully qualified Place title into a hierarchy. (Hopefully testing for duplicates for each enclosing hierarchical level.)

In the future, someone might write a parser that interprets a Gazetteer entry or other structured place format or a natural language parser (like FamilySearch's that extracts any text recognizable as a Place and offers a list).

FamilySearch's parser allows the user to paste the same chunk of text into the 6 fields for a new person (birth date, birth place, death date, death place, burial date and burial place) and then it offers a drop down menu of text recognized as appropriate for the type of field. So the following text would offer 2 dates or 3 places when pasted from a copy buffer of the Mary Ball Washington FindAGrave memorial: "30 Nov 1708
Lively, Lancaster County, Virginia, USA
Death 26 Aug 1789 (aged 80)
Spotsylvania Courthouse, Spotsylvania County, Virginia, USA
Burial Kenmore Plantation and Gardens
Fredericksburg, Fredericksburg City, Virginia, USA"

@Nick-Hall Nick-Hall marked this pull request as draft July 17, 2023 22:24
@Nick-Hall Nick-Hall removed this from the v5.2 milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.