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

Improvements related to selection of feature(s) on map when modal tools are active #217

Merged
merged 7 commits into from Aug 23, 2017

Conversation

Projects
None yet
2 participants
@nprigour
Contributor

nprigour commented Mar 8, 2017

The specified contribution enables selection of a feature that a modal tool action will apply in case of overlapping feature. Moreover it provides some extra preference options to control this selection behavior. All improvements are listed below (see also screenshots in attached zip file):

  1. enables right click popup menu on map when selection modal tools are active
  2. enables selection of a feature during delete, edit, selection etc. on map when overlapping features (or features within a specified selection radius) are found, by appropriate popup menu (popup displays by default the FID value unless a default feature attribute is specified in the Tool Preferences page)
  3. Extra preference options in general Tool preference page
  4. Extra Preference page related to Deletion tool
  5. Allows for confirmation of a feature deletion action (controllable via preference option)

toolsImprovements.zip

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

Improvements related to selection of feature on map during modal tools
activity

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

This comment has been minimized.

Contributor

fgdrf commented Jul 18, 2017

During Eclipse Hackathon I had a look at and have a question regarding configuration page which Attribute of a feature is used: Is it a global configuration which is used for different Layers with different FeatureTypes or layer-specific? In addition, is the configured attribute name used case-sensitive (i guess from databases the attributes are uppercases whereas other ressources can handle upper and lower cases)?

Please correct me if I'm wrong, this pull request changes the behavior for Features for the selected Layer. With other words : Does it query all layers and provides features from different layers or just from the selected?

@nprigour

This comment has been minimized.

Contributor

nprigour commented Jul 19, 2017

Hi @fgdrf ,
I suppose you are referring to the default feature attribute that I have added in the top level Tool preference page. This applies generally to all available layers of a map. Below I will try to better explain the reason of introducing it:
This is a preference options that is related to the ability of displaying information for overlapping features during the various actions performed in udig (also addressed by this pull request). Until know when you click on a map and there are overlapping/collocated features, it is not possible to select to choose which one to select (I think one is selected based probably on the most recent fid;;;). With this pull request, when you click on the map and there are collocated/overlapping features a popup appears that displays the available features at the point of click. Therefore now you have a choice on what to select.
However when I initially developed this improvement the popup info used by default the features FID value. This in most cases is a kind of sequence number that does not have a real value for the user. Therefore I introduced this new preference property (FEATURE_ATTRIBUTE_NAME = "featureAttributeName") which provides the ability to override this behavior.
If you set this value then before the popup is displayed for a specific layer an attempt will be made to retrieve the provided attribute. If a value either than null (which may be an indication that no such attribute is present) or empty String is returned it will be used in the popup otherwise the FID will be used.
Therefore to directly answer your questions:

  • it is a configuration options that applies globally
  • no change in the behavior of Features occurs. As explained it has to do only with visualization/display purposes
  • There is no query to any layer. Just during a mouse click on the map while a selection,edit or delete tool is active the specified attribute name (if present) is retrieved from the features of the selected layer that match the FEATURE_SELECTION_RADIUS criteria.
  • If a layer (featureType) does not have this attribute then the call to feature.getAttribute(...) will always return null and the FID will be displayed.
for (final SimpleFeature feat : features) {
MenuItem item = new MenuItem(menu, SWT.PUSH);
//SimpleFeature feature=iter.next();
boolean hasNameAttrib = feat.getAttribute(ATTRIBUTE_NAME) != null

This comment has been minimized.

@fgdrf

fgdrf Jul 19, 2017

Contributor

I guess SimpleFeature.getAttribute() is case-sensitive regarding access featureType definition. E.g. if "NAME" is configured but attribute is "Name" feat.getAttribute(ATTRIBUTE_NAME) would return null.

Maybe its worth the effort to imlement case-insensitve access right here (by evaluating featureType attribute names toLowerCase() and find an appropriate ATTRIBUTE_NAME.toLowerCase())). What Do you think?
it worth to think about case-insensitive

This comment has been minimized.

@nprigour

nprigour Jul 20, 2017

Contributor

I will have a look at it. However it will need to iterate all featureType attribute names and perform comparisons.

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Jul 19, 2017

@nprigour Many thanks! Whenever a featureType has a attribute configured in the preferences page the value of features attribute is shown, right? If there is no attribute like configured, fid is used to present for the user.

BTW, cool to have right mouse-functionalliy for selection tools as well!

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Jul 19, 2017

@nprigour Would you be kind and add another section to user-Documentation? I guess Selection Tools.rst would be a good page to describe behavior and configuration option

provide case insensitive lookup of ATTRIBUTE_NAME property
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>

@fgdrf fgdrf added the improvement label Jul 25, 2017

* @return a list of actual propertyName (case sensitive). If a propertyName is not present
* then it is skipped from the list
*/
public static List<String> getActualPropertyName(final SimpleFeatureType featureType, final List<String> propertyNames) {

This comment has been minimized.

@fgdrf

fgdrf Jul 25, 2017

Contributor

Looks like this function is never used. Do we need at for this feature?

This comment has been minimized.

@nprigour

nprigour Jul 25, 2017

Contributor

No, it is a util function for possible future usage. after all this class contains util feature functions

PlatformGIS.syncInDisplayThread(new Runnable() {
@Override
public void run() {
final String attribName = FeatureUtils.getActualPropertyName(

This comment has been minimized.

@fgdrf

fgdrf Jul 25, 2017

Contributor

Looks like it would take the first attribute that matches, in case of FeatureType DataUtilities.createType("testType", "geometry:Polygon,name:String,timestamp:java.util.Date,NAME:String");

it would return values of the first attribute name (because its in the definition before NAME), wouldn't it?

This comment has been minimized.

@nprigour

nprigour Jul 25, 2017

Contributor

probably yes but how usual it is to have attributes with same name but different capitalization within the same featureType?

@Override
public void run() {
final String attribName = FeatureUtils.getActualPropertyName(

This comment has been minimized.

@fgdrf

fgdrf Jul 25, 2017

Contributor

I created some test cases (see commit nprigour@476378a), is this the expected behavior (especially the last test method)?

This comment has been minimized.

@nprigour

nprigour Jul 26, 2017

Contributor

@fgdrf , I modify FeatureUtils.getActualPropertyName(..) to perform first a strict case sensitive checking for attribute name before resorting to a case insensitive one.

nprigour added some commits Jul 26, 2017

update documentation related to modal tool improvements
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
modify getActualPropertyName of FeatureUtils to check 1st for an
exactmatch before resorting to case insensitive comparison

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

This comment has been minimized.

Contributor

fgdrf commented Jul 26, 2017

@nprigour Please have a look at the pull-request nprigour#2 which contributes test cases and java 1.6 compatible behavior.

@nprigour

This comment has been minimized.

Contributor

nprigour commented Jul 26, 2017

@fgdrf after merging your test pull request to this one it seems ip-validation failed due to inproper sign-off. I performed the merge directly from github web interface and expected to take my signature but something seems wrong. Can you advice? Should I revert the commit and try the merge locally from eclipse IDE?

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Jul 26, 2017

Looks like IP-validation fails because of the merge commit. Can you try to reset to commit a84f39f (--hard)

Afterwards I can create another pull request we can try to rebase onto your branch (avoids merge commit) or even squash the commit. You can find this options on the pull request dialog ;)

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Jul 26, 2017

Let walk through this, I have seen that the merg commit has invalid committer id noreply@github.com

I expect, if we can get rid of this merge ip validation would be fine, please try this on your local branch - if you don't have other changes made anymore!:

git reset a84f39f #resets to commit we can start again from

now local working copy should have changes while the last commit is a84f39f.

Now cleanup with git checkout -- . #cleanup local workingcopy (git reset --hard would work too

Afterwards, you can either cherry-pick the commit 98b6655 (my changes) and push your branch to remote branch again using the force-option:

git push -f GeneralModalToolsImprovements

which re-writes history on remote-branch.

However, we can chat (irc://us.freenode.net/udig) tomorrow morning (CEST) and try to fix it. We will make it!

* it is skipped from the list
* @throws IllegalArgumentException if featureType and/or propertyNames is null
*/
public static List<String> getActualPropertyName(final SimpleFeatureType featureType,

This comment has been minimized.

@fgdrf

fgdrf Jul 26, 2017

Contributor

What is this function for? Haven't found any caller in workspace ..
Is it ok for you if we can remove it from this pull-request?

This comment has been minimized.

@nprigour

nprigour Jul 26, 2017

Contributor

As explained in a previous comment it is not current used but I included there since FeatureUtils is a generic util class and it might be handly for somebody in the future. After all it is a more generialized version of the one I am using. Checking udig code, it seems there are also similar cases in other util classes within udig project

nprigour and others added some commits Jul 27, 2017

fix code to protect from NPE in case attribValue for an existing
attributeName is null

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
added TestCases for FeatureTypeUtils
 * made parameter validation java 1.6. compatible (throws
 IllegalArgumentException)
 * formatted code using extras/org.locationtech.udig.dev/codeformatter.xml

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>
@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Jul 27, 2017

@nprigour Awesome, it looks like you managed to get rid of this merge commit! Well done!

@nprigour

This comment has been minimized.

Contributor

nprigour commented Jul 27, 2017

@fgdrf ,
I revert and cherry picked your commit. It should bed OK now.
Note that I did a further modification of the code to protect for NPE when retrieving the value to be displayed.

/**
* The radius to be used during UI single feature selection search like commands (select, edit etc.).
*/
public static final String FEATURE_SELECTION_RADIUS = "featureSelectionRadius"; //$NON-NLS-1$

This comment has been minimized.

@fgdrf

fgdrf Jul 31, 2017

Contributor

Is it your intention to define a radius in pixel or unit of measure of current CRS? I assume in pixel and if so, maybe we can make it much clearer:

  • FEATURE_SELECTION_RADIUS_IN_PIXEL
  • label name Selection radius (in Pixel)

Makes it easier to read code and varification of own expectations ;)

DeleteTool_confirmation_title = Confirm Delete
DeleteTool_status = Click on the feature you wish to delete
DeleteTool_warning = Select a different layer and try again
DeleteToolPreferences_description = Delete Tool preferences
DeleteToolPreferences_Delete_Radius = Delete Radius

This comment has been minimized.

@fgdrf

fgdrf Jul 31, 2017

Contributor

Like for Selection Tool Deletation Tool should state the units Deletion Radius (in Pixel) I assume here either

@@ -42,6 +46,10 @@
*/
public class SetEditFeatureCommand extends AbstractCommand implements UndoableMapCommand {
// The size of the box that is searched. This is a 7x7 box of pixels by default.
private int SEARCH_SIZE = Platform.getPreferencesService().getInt(
ProjectUIPlugin.ID, PreferenceConstants.FEATURE_SELECTION_RADIUS, 7, null);

This comment has been minimized.

@fgdrf

fgdrf Jul 31, 2017

Contributor

Is 7 the default, if nothing is set in PreferenceStore? I guess it makes sense to define a global Default which was:

  • store.setDefault(PreferenceConstants.P_DELETE_TOOL_RADIUS, 6); or even
  • store.setDefault(PreferenceConstants.FEATURE_SELECTION_RADIUS,6);
    in PreferencesInitializer but is different to this one.
super.setActive(active);
DELETE_SEARCH_SIZE = Platform.getPreferencesService().getInt(
EditPlugin.ID, PreferenceConstants.P_DELETE_TOOL_RADIUS, 6, null);

This comment has been minimized.

@fgdrf

fgdrf Jul 31, 2017

Contributor

Can we use global definition of Defaults here as well?

@@ -31,6 +31,8 @@
/**
* Selects and drags single features.
*
* @deprecated {@link ArrowSelectionWithPopup} should be used instead

This comment has been minimized.

@fgdrf

fgdrf Jul 31, 2017

Contributor

Thats great to keep it in the code base, SDK-Users may still use this action in their application. Thinking about moving selection tools into a separate bundle to make it possible for SDK-Users to choose preferred selection tools. The "new" bundle should provide its own preferences too.

What do you think about creation a bundle

  • org.locationtech.udig.tool.select.point.simple for old code
  • org.locationtech.udig.tool.select.point.featureselect for your contribution.

IMHO its a valid requirement if users of uDig SDK can choose to create their prefered application. In addition I see few other selection bundles (bbox, AOI, etc) not everybody wants to include per default.

However, my suggestions is to create another pull request that only has changes for deletion tool (radius and confirmation dialog, etc). With this pull request we can move on with the selection tool changes.

Now I'm wondering a bit about AbstractModalTool changes. Only ArrowSelectionWithPopup uses SELECTION_SEARCH_SIZE Please help me to understand..

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Aug 13, 2017

Just created an issue to seperate selection tools into different bundles : #249

Collaboration on Selection/Delete Tools (#4)
* added tests for FeatureUtils.getActualPropertyName

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* updated javadoc, optimized attributes access

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* updated selection prefs behavior

 * added link to general tool preferences page
 * selection radios config on selection preferences page
 * fixed prefs link in editor for selection tools
 * tool uses correct prefs while config changed in between (by user) and tool is still active

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* unified pref pages and tool behavior (delete)

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* unified naming and added javadoc

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* delete confirmation dialog uses feat attribute

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* fixed prefs link in editor for feature delete tool

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* fixed NPE

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* delete confirmation shows attribute and id

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* fixed imports

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* added moved files

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* fixed imports of SelectionToolPreferencePage

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* [user-doc] updated tool preferences

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* unified preferences and docs

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* applied DeleteTool attrib improvement

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* cleanup duplicated preferences options

* removed P_SHOW_ANIMATIONS from Edit Performance Preferences
* updated user docs Edit Tool Performance Preferences

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* used defaults for commands, updated doc image

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

* removed unused NLS keys

Signed-off-by: Frank Gasdorf <fgdrf@users.sourceforge.net>

@fgdrf fgdrf added this to the uDig-2.0.0 milestone Aug 23, 2017

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Aug 23, 2017

Anything else you like to to on this branch or can I start rebasing and merging this improvement?

@nprigour

This comment has been minimized.

Contributor

nprigour commented Aug 23, 2017

Nothing from my side. You can proceed with merging.

@fgdrf fgdrf merged commit 9d1d8f6 into locationtech:master Aug 23, 2017

1 check passed

ip-validation
Details
@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Aug 23, 2017

@nprigour Thank you very much for this contribution and the smooth collaboration!

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