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

improve FeatureTypeEditor to provide fields for setting attributes restrictions #233

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@nprigour
Contributor

nprigour commented May 26, 2017

The pull request improves FeatureTypeEditor UI (launched during 'create feature type' or 'add feature type' operations) so that:

  • the attribute length can be set (if applicable)
  • whether the attribute can be nillable or not.

FeatureTypeEditor improvement.pdf

Signed-off-by: Nikolaos Pringouris nprigour@gmail.com

improve FeatureTypeEditor to provide fields for setting some
restrictions

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Jun 2, 2017

Tested additional attributes and I'm wondering about null Allowed check. I set name to false and I could create features without a name while the attribute lenght is validated in the table view

name restriction [ length([.]) <= 3 ] not met by: 123123

I haven't got any feedback for the null-check on edits and on commit.

Question : How can we avoid confusing users if e.g. the user selects integer as type and inputs 99 fir attribut lenght. The second configuration doesn't really make sence because of the type itself:

As value allowed:Integer if the user types 12123123123123

It seems to me, that dependent of the selected type the options may be different. I guess this dialog should be redesigned a bit, e.g. splitted view with attributes on the top and in a detail the possible configuration dependet of the selected type.

@nprigour What do you think?

@nprigour

This comment has been minimized.

Contributor

nprigour commented Jun 4, 2017

  • about null Allowed check:
    Are you sure that the name is set to null or it is just set to an emty String? I think the latter is the case.

  • about the attribute lenght:
    maybe it is not applicable everywhere (i.e. I have disabled it in geometry types) and therfore we can disable it for some other types also. However the attribute lenght field provides just an extra level of constain (the 1st one imposed by the defined attribute type) so I do not really think we should really care if for an integer field someone provides a large length that overcomes max int value. The type restriction will apply first and a large value will not be accepted anyway.

The enhancement just provides the ability to define restrictions during creation of featureTypes not to impose them when creating/editing features. The result will largely depend on whether they featureType refers to a shapefile layer or a jdbc layer. You can check the constraints by selecting a layer and then right clicking to Properties-->Summary-->Feature Type

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Mar 26, 2018

Are you sure that the name is set to null or it is just set to an emty String? I think the latter is the case.

I guess we need to check. I expected a null-value rasther then an empty string is set if the user doesn't input anything/ leaving unset.

The enhancement just provides the ability to define restrictions during creation of featureTypes not to impose them when creating/editing features.

Is this a bug in uDig? Maybe I miss something but I expected a validation/check on feature creation if a feature type has such restictions ..

Would you please help me to understand, why and how this improvement helps users? What a user cannot do, if the feature isn't present?

Many thanks for clarification!

@nprigour

This comment has been minimized.

Contributor

nprigour commented Mar 27, 2018

Hi @fgdrf
As already mentioned the provided pull request provides some ability to define restrictions when creating a SimpleFeatureType either backed by an shaprefile or against the database. However the enhancement has to do only with the creation of a new FeatureType it does not address issues related with the creation or editing of Features in the shapefile or database layer.
For example if we create a new FeatureType against a Mysql DB and define a String field with max lenght 20 then when actual creation takes place the corresponding DB column will be created as varchar2(20). On the other hand creating an Integer fie;d with length 5 while it creates the constrain in athe shapefile case it does not have a real sense for the DB case since the creation will be done as decimal.
Now imposing of the constraints is the responsibility of the shapefile or DB and can be assisted by the editor view used for editing. Currently udig has 2 main editor views:

  1. the table view --> if constraint are define like max length and you try to input entries with larger number of chars/digits then a red background indicates that this not possible (see figure below an example where in a field with lenght 20 I have put 21 chars
    default
  2. the FeatureEditor --> here there is no background highlight and probably not an apriori validation. You can input fields with larger length than the constrain set and the actual validation is left to the underline dataStore (.e in the DB case a DB eror is thrown while in the shapefile case a data truncation is performed)

Regarding null values indeed I checked it. The values set if you left the a String field empty is empty string and not null and that is the reason it accepts the value. Also both editors are not capable to handle/accept null values in numeric fields or Date types. However all these are really editing issues and not related to the present pull request. If you want you can create another issue on that and address it seperately.

@fgdrf fgdrf added the feature label Mar 27, 2018

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Mar 27, 2018

For example if we create a new FeatureType against a Mysql DB and define a String field with max lenght 20 then when actual creation takes place the corresponding DB column will be created as varchar2(20).
On the other hand creating an Integer fie;d with length 5 while it creates the constrain in athe shapefile case it does not have a real sense for the DB case since the creation will be done as decimal.

This was the missing puzzle that helps a lot!

However all these are really editing issues and not related to the present pull request. If you want you can create another issue on that and address it seperately.

Great to sort out. Now my understanding is that this is an additional feature to support restictions for several data stores that has support for these. Editing features could be improved on the other hand to verify created features before the user tries to commit invalid features which fails on another level

Many thanks

@fgdrf fgdrf added this to the uDig-2.0.0 milestone Mar 27, 2018

@@ -807,4 +891,26 @@ public void setErrorMessage( String errorMessage ) {
errorDecorator.showHoverText(errorMessage);
}
/**
* @param editElement

This comment has been minimized.

@fgdrf

fgdrf Mar 27, 2018

Contributor

It would be great to see a vaild javadoc and only one @return statement ;)

However, does it make sense to check restrictionName for null and convert toLowerCase() for comparision as well?

This comment has been minimized.

@nprigour

nprigour Mar 30, 2018

Contributor

OK this was addressed as well as some other minor issues

builder.length(MAX_ATTRIBUTE_LENGTH).add(Messages.FeatureTypeEditor_defaultNameAttributeName, String.class);
builder.add(Messages.FeatureTypeEditor_defaultGeometryName, LineString.class);
//no need to provide a default length since length can be specified via the UI
//builder.length(MAX_ATTRIBUTE_LENGTH).add(Messages.FeatureTypeEditor_defaultNameAttributeName, String.class);

This comment has been minimized.

@fgdrf

fgdrf Mar 27, 2018

Contributor

Is the constant MAX_ATTRIBUTE_LENGTH referenced somewhere else or coult it be deleted? IMHO the comment on the constant indicates that it is a specific value for a specific database which might not make any sense for other storage types (file/other databases)

This comment has been minimized.

@nprigour

nprigour Mar 30, 2018

Contributor

No it is not used. Initialy it was used for setting a default length for the lenght of an attribute but this become obsolete since the new column for etting lenght was intriduced

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Mar 27, 2018

@nprigour Since it is a new feature I would appreciate to extend Create Section in Docs. From my point of view the explaination of how restictions would work would be great to see here as well as a screenshot of the dialog like seen in the pdf file

nprigour added some commits Mar 29, 2018

fix javadoc and minor improvements/corrections
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
update docs section
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
@nprigour

This comment has been minimized.

Contributor

nprigour commented Mar 30, 2018

user docs updated

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 4, 2018

Looks like FeatureTypeEditor has som conflicts we need to resolve. @nprigour would you please have a look at, I cannot resolve it with my cellphone ;)
Can you help here? Thanks again!

resolve conflicts
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 5, 2018

it should be OK for merge now.

Note: I did not enabled skipped FeatureEditorTest since it will result in another conflict. You can do it after merge of this to master branch

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 5, 2018

Looks like I have to resolve conflicts manually and I need my developer env to do so. Going to merge it next Wednesday! Stay tuned and many thanks for this new feature!

@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 5, 2018

strange because yesterday I tried to merge this branch to the master one (in my local working directory) and it succeeded So I assumed that there will be no conficts also when merging the pull request

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 5, 2018

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 10, 2018

merged to master

@fgdrf fgdrf closed this Apr 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment