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 editing of features #274

Merged
merged 3 commits into from Apr 15, 2018

Conversation

Projects
None yet
2 participants
@nprigour
Contributor

nprigour commented Apr 12, 2018

The FeaturePropertySource & AttributePropertyDescriptor enhancements include:

  • fix certain object casts
  • send edit commands only in case of actual value change
  • handling of empty String values as null
  • DateTimeCellEditor added
  • NumberCellEditor added that can hnadle setting of null values for numbers

derived from splitting of #254

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

@fgdrf

@nprigour Thanks for this improvement! I'm wondering if you could address minor changes I commented.

Cool thing to get only valid Values and even null values applied to feature attributes. I'm asking myself if there is a need to have a difference in behavior to set an empty value rather than a null value...

try {
return convertToNumber();
} catch (NumberFormatException e) {
e.printStackTrace();

This comment has been minimized.

@fgdrf

fgdrf Apr 13, 2018

Contributor

This causes errors logged in console / error view and the value is cleared for feature. I tested to override isValueValid which leaves the feature untouched in case of any NumberFormatException. With this change the try/catch block in doGetValue isn't required anymore..

    @Override
    public boolean isValueValid() {
        try {
            Number number = convertToNumber();
            return (number != null);
        } catch (NumberFormatException e) {
            return false;
        }
    }
/**
* Test Cell Editor
*
* @author Jesse

This comment has been minimized.

@fgdrf

fgdrf Apr 13, 2018

Contributor

Your credits, use here @author Nikolaos Pringouris <nprigour@gmail.com> as well ;)

assertNull(editor.getValue() );
}
private void runTest( Object value, Object value2, Class<? extends Number> class1 ) {

This comment has been minimized.

@fgdrf

fgdrf Apr 13, 2018

Contributor

I'd like to suggest to rename to something like assertSetAndGetTwoValues or even split it into two calls of assertSetAndGetValue.

I tested with a Parametrized Test, goint to create a pull request for your branch ..

This comment has been minimized.

@nprigour

nprigour Apr 13, 2018

Contributor

actually for that I just adopted the same naming approach as used for BasicTypeCellEditorTest

* Test Cell Editor
*
* @author Jesse
* @since 1.1.0

This comment has been minimized.

@fgdrf

fgdrf Apr 13, 2018

Contributor

version 2.0.0 ;)

assertNull(editor.getValue() );
//not parsable number
editor.setValue("aa");

This comment has been minimized.

@fgdrf

fgdrf Apr 13, 2018

Contributor

this might be obsolete if isValueValidis implemented...

public void setDateFormatter(DateFormat format) {
this.format = format;
}

This comment has been minimized.

@fgdrf

fgdrf Apr 13, 2018

Contributor

Override isValueValid here as well to avoid clearance of the feature attribute value in case of ParseException

@fgdrf fgdrf added the improvement label Apr 14, 2018

@fgdrf fgdrf added this to the uDig-2.0.0 milestone Apr 14, 2018

@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 14, 2018

Hi @fgdrf ,

I have addressed most of your comments/requests with the exception of renaming of runTest (I have explained why.
Moreover in addition to the requested changes I performed:

  • some formatting of code and
  • changes in TableView class to allow correct setting of null values during editing of a feature in table view
@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 14, 2018

Hi @fgdrf ,

I have addressed most of your comments/requests with the exception of renaming of runTest (I have explained why.
Moreover in addition to the requested changes I performed:

  • some formatting of code and
  • changes in TableView class to allow correct setting of null values during editing of a feature in table view

Besides all the above and before merging the pull request I would like your opinion if it is worthy to perform also some code refactoring to gather for exapmle all cell editors in a separate package witihin orgo.locationtech.udig.ui plugin. What do you think?

@fgdrf

fgdrf approved these changes Apr 14, 2018

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 14, 2018

some formatting of code

I'm wondering about your format settings. Have you configured codeformatter.xml settings for your workspace (Preferences —> Java —> Code Style —> Code Formatter)?

exception of renaming of runTest ..

I'm fine with it althought it is not convenient naming such methods. It was just a hint to improve codebase while other classes still might have "special" style ;) And, especially in this case, its copied or used as template which makes it even worse.

Besides all the above and before merging the pull request I would like your opinion if it is worthy to
perform also some code refactoring to gather for exapmle all cell editors in a separate package
witihin orgo.locationtech.udig.ui plugin. What do you think?

Good Idea and I'd like to suggest to seperate it from this pull request. We can collaborate on your suggested refacturings (sub-packaging).

@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 14, 2018

Yes I use the specified code formatter. However occasionally it may occur that during editing formatting does not apply and sometimesI need to explixitly select format from the eclipse editor context menu.

Ok no objection to handle refactoring to a subpackage in a separate pull request.

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 14, 2018

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 14, 2018

Can you try to select all lines in at least new files you created and Format code again? Do you have configures some specific save Actions (Preferences -> Java -> Editor -> Save Actions)?

@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 14, 2018

re-apply formatting in:

NumberCellEditor.java
NumberCellEditorTest.java
DateTimeCellEditor.java
DateTimeCellEditorTest.java
DateTimePickerDialog.java

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 15, 2018

Can you merge latest changes from master into your branch and push the btranch again? The build didn't finished since an old WMS url let it timeout (https://ci.eclipse.org/udig/job/uDig-PR/2/) which is fixed on master already ;)

many Thanks

@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 15, 2018

does this wms related fix conflicts with the present pull? It seems not. Do you want to just merge and push or rebase and force push?

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 15, 2018

nprigour added some commits Apr 12, 2018

Improvements related to editing of features
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
improve validation, correct author and version comments and fix editing
in Table view

Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
fix formatting in new files
Signed-off-by: Nikolaos Pringouris <nprigour@gmail.com>
@nprigour

This comment has been minimized.

Contributor

nprigour commented Apr 15, 2018

rebase succesfully applied

@fgdrf

This comment has been minimized.

Contributor

fgdrf commented Apr 15, 2018

@fgdrf fgdrf merged commit 1afd6e5 into locationtech:master Apr 15, 2018

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