-
Notifications
You must be signed in to change notification settings - Fork 424
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
base: master
Are you sure you want to change the base?
Conversation
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. |
@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.). |
@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. |
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. |
gramps/gen/lib/placetype.py
Outdated
""" | ||
Place Type class. | ||
|
||
This class is for keeping information about place typess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "typess"
gramps/gen/lib/placetype.py
Outdated
|
||
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; |
There was a problem hiding this comment.
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?
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: For me, this means making a messy product even worse. Why can't we clean things first? |
I agree. The approach I outlined as a result of the user consultation listed the following requirements for place type and groups:
and the following requirements for rules:
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. |
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. |
ok, and done if you comment out the types added for testing.
done
done
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.
done
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...
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. |
Great work! It seems you have made progress.
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. |
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. 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. |
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. |
@Nick_H did you post your glade files anywhere so I could start with those?
|
Closing the Place Types dialog gives me an error: 159982: ERROR: grampsapp.py: line 150: Unhandled exception 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. |
I get another error when I try to open the Place Format Editor: 52486: ERROR: grampsapp.py: line 150: Unhandled exception Both errors don't appear when I use an SQLite database. |
Would love to see a 'grouping' too. eg, inherited from gedcom types or American model :
|
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. |
@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. |
@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. |
@ennoborg @romjerome Gentelment, I'm not too clear on what you mean in your comments.
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." |
Issue #11335
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. 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 |
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:
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.