-
Notifications
You must be signed in to change notification settings - Fork 96
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
base: main
Are you sure you want to change the base?
Conversation
the new ValidatedTuple)
…down after the timeout
This is cool, especially your context manager to dispose the ui after x sec. Don't forget a contribution to CHANGES.txt |
I think this probably needs to wait for a traits release containing the |
# Conflicts: # traitsui/tests/editors/test_tuple_editor.py # traitsui/tests/test_tuple_editor.py
@corranwebster this is now ready to review |
Current coverage is 53.32%@@ 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
|
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 |
# 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 |
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.
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()
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.
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.
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.
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): |
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.
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)
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.
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.
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.
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 |
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.
Any reason why fvalidate
instead of just validate
or validator
?
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.
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
I am tempted to merge this as-is. Nothing above is a show-stopper, and it would be good to have this finished. |
@corranwebster, this PR still needs so fixes related to the ui behavior. |
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:
normal values
invalid field
invalid tuple (x[0] > x[1])
note: Tests require #enthought/traits#205