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

[WIP] Feature/advanced tuple editor #182

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

itziakos
Copy link
Member

@itziakos itziakos commented Oct 7, 2014

This PR augments the TupleEditor to support extra validation provided by the user in the Editor factory or defined in the trait (see ValidatedTuple PR #enthought/traits#205).

example:

>>> from traits.api import Int, HasStrictTraits, Tuple
>>> from traitsui.api import View, Item, TupleEditor
>>> class Model(HasStrictTraits):
...     value_range = Tuple(Int(0), Int(1))
...     view = View(Item('value_range', editor=TupleEditor(fvalidate=lambda x: x[0] < x[1])))
... 
>>> model = Model()
>>> model.configure_traits()
>>> 

image
normal values

image
invalid field

image
invalid tuple (x[0] > x[1])

note: Tests require #enthought/traits#205

@jonathanrocher
Copy link
Contributor

This is cool, especially your context manager to dispose the ui after x sec. Don't forget a contribution to CHANGES.txt

@corranwebster
Copy link
Contributor

I think this probably needs to wait for a traits release containing the ValidatedTuple before its safe to merge this.

@itziakos
Copy link
Member Author

itziakos commented Jun 6, 2016

@corranwebster this is now ready to review

@codecov-io
Copy link

codecov-io commented Jun 6, 2016

Current coverage is 53.32%

Merging #182 into master will increase coverage by 1.72%

@@             master       #182   diff @@
==========================================
  Files           115        111     -4   
  Lines         11401      11090   -311   
  Methods           0          0          
  Messages          0          0          
  Branches       1868       1784    -84   
==========================================
+ Hits           5883       5913    +30   
+ Misses         4994       4696   -298   
+ Partials        524        481    -43   

Powered by Codecov. Last updated by 536ff2c...ef2a78d

@mdickinson
Copy link
Member

I wonder whether both fields should have a red background in the case that the pair of values fails validation. It's not obvious right now why one field is red but the other not. With the example from the initial comment (and validation replaced by fvalidate), I can achieve all three of the following states (the first text box is active in all three cases).

screen shot 2016-06-07 at 09 40 36
screen shot 2016-06-07 at 09 40 52
screen shot 2016-06-07 at 09 41 01

# Conflicts:
#	traitsui/editors/tuple_editor.py
#	traitsui/tests/_tools.py
#	traitsui/tests/editors/test_tuple_editor.py
# 'SimpleEditor' class:
#-------------------------------------------------------------------------
# The validation function to use for the Tuple. If the edited trait offers
# already a validation function then the value of this trait will be
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the other way around (unless you have a particular use-case in mind). Because one HasTraits class can potentially have many different views, you generally want thing specified in the editor to override things specified in traits.

Although not exactly analogous, compare with (note that the label in the Item wins):

from traits.api import HasTraits, Float
from traitsui.api import View, Item


class Test(HasTraits):

    x = Float(label='Variable x')

    view = View(Item('x', label='Var x'))

t = Test()
t.edit_traits()

Copy link
Member Author

@itziakos itziakos Oct 13, 2016

Choose a reason for hiding this comment

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

As you say it is not analogous. Since there is no restriction on the label. For example if the Editor validation overrides the trait validation it can potentially result in an invalid value been forced into a trait which will throw an exception.

Copy link
Member Author

@itziakos itziakos Oct 13, 2016

Choose a reason for hiding this comment

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

Thinking about it a little more, maybe a compromise would be the union of the trait validation and editor validation.

At the end the problem is about who is responsible for ultimate constrains in the values (not the representation) the traits model or the traits ui view

new_value = tuple(
getattr(self, 'f{0}'.format(i)) for i in range(self.fields))
if self.fvalidate is not None:
if self.fvalidate(new_value):
Copy link
Contributor

Choose a reason for hiding this comment

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

A more powerful way of validating is to have the validator have a chance to modify the values to something that might work (see the way that Traits validators work). So something like:

try:
    editor.value = self.fvalidate(new_value)
except TraitError:  # or something else appropriate
    for i in range(self.fields):
        setattr(self, 'invalid{0}'.format(i), False)

Copy link
Member Author

@itziakos itziakos Oct 13, 2016

Choose a reason for hiding this comment

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

I am not convinced. If you want the validation to change the value then use a custom trait that preforms the coersion. Normal traits like Float, Range and Int do not covert the values. A number of editors already provide a format function to support conversions.

Copy link
Member Author

@itziakos itziakos Oct 13, 2016

Choose a reason for hiding this comment

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

Also the fact that the validate function of some traits changes the actual value is an internal implementation/optimization. For example the coercing traits CFloat and other will do the validation at the same time as coercion. So by returning the coerced value one does not have to perform the coercion again. But this does not happen in all the traits just those that their Name/Definition imply a coercion.

The generic trait behavior e.g. a Property has separate functions for get, set and validation. Where conversion does not have to happen in validation but in set

# The validation function to use for the Tuple. If the edited trait offers
# already a validation function then the value of this trait will be
# ignored.
fvalidate = Callable
Copy link
Contributor

@corranwebster corranwebster Oct 13, 2016

Choose a reason for hiding this comment

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

Any reason why fvalidate instead of just validate or validator?

Copy link
Member Author

@itziakos itziakos Oct 13, 2016

Choose a reason for hiding this comment

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

It has been a while... So initially I had it as validator but then I changed it to fvalidate to be similar to the Property fget, fset, fvalidate

# Conflicts:
#	traitsui/tests/_tools.py
#	traitsui/tests/test_tuple_editor.py
@corranwebster
Copy link
Contributor

I am tempted to merge this as-is. Nothing above is a show-stopper, and it would be good to have this finished.

@itziakos itziakos changed the title Feature/advanced tuple editor [WIP] Feature/advanced tuple editor Oct 10, 2017
@itziakos
Copy link
Member Author

@corranwebster, this PR still needs so fixes related to the ui behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants