Skip to content

Add RangeEditor support for format_func #1684

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.

Sign up for GitHub

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

Merged
aaronayres35 merged 15 commits into from
Jun 29, 2021
Merged

Add RangeEditor support for format_func #1684

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
dd82eec
add a currently failing test
aaronayres35 Jun 11, 2021
cf42ce4
remove call to configure_traits
aaronayres35 Jun 11, 2021
d25a17f
make test pass on qt by using the string_value method on the factory
aaronayres35 Jun 11, 2021
fc9ae39
make same fix on wx
aaronayres35 Jun 11, 2021
3032065
redefine string_value method on RangeEditor to preserve behavior. We…
aaronayres35 Jun 11, 2021
017714b
add a test for use of format trait to ensure we aren't changing behavior
aaronayres35 Jun 11, 2021
7383f01
fix test and string_value method (missed one format_str)
aaronayres35 Jun 11, 2021
9ad59b8
add news fragment
aaronayres35 Jun 11, 2021
bb7a3c4
deprecate format trait and make it an alias for format_str
aaronayres35 Jun 21, 2021
f4fc4b0
add deprecation comment on wx
aaronayres35 Jun 21, 2021
a7304b0
add a test demonstrating behavior change
aaronayres35 Jun 22, 2021
cd26554
call string_value on the editor as this ends up calling the method on…
aaronayres35 Jun 22, 2021
68c42f1
Apply suggestions from code review
aaronayres35 Jun 28, 2021
d618dde
update news fragment
aaronayres35 Jun 28, 2021
2bf3fa5
add comment refering to new issue
aaronayres35 Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/releases/upcoming/1684.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add RangeEditor support for format_func (#1684)
25 changes: 23 additions & 2 deletions traitsui/editors/range_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

""" Defines the range editor factory for all traits user interface toolkits.
"""
import warnings

from types import CodeType

Expand Down Expand Up @@ -64,8 +65,11 @@ class RangeEditor(EditorFactory):
#: The name of an [object.]trait that defines the high value for the range
high_name = Str()

#: Formatting string used to format value and labels
format = Str("%s")
# set format_str default
format_str = Str("%s")

#: Deprecated: Formatting string used to format value and labels
format = Property(Str, observe='format_str')

#: Is the range for floating pointer numbers (vs. integers)?
is_float = Bool(Undefined)
Expand Down Expand Up @@ -194,6 +198,23 @@ def _get_low_high(self, ui):
# -------------------------------------------------------------------------
# Property getters.
# -------------------------------------------------------------------------

def _get_format(self):
warnings.warn(
"Use of format trait is deprecated. Use format_str instead.",
DeprecationWarning,
stacklevel=2,
)
return self.format_str

def _set_format(self, format_string):
warnings.warn(
"Use of format trait is deprecated. Use format_str instead.",
DeprecationWarning,
stacklevel=2,
)
self.format_str = format_string

def _get_simple_editor_class(self):
""" Returns the editor class to use for a simple style.

Expand Down
18 changes: 8 additions & 10 deletions traitsui/qt4/range_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class SimpleSliderEditor(BaseRangeEditor):
#: High value for the slider range
high = Any()

#: Formatting string used to format value and labels
#: Deprecated: This trait is no longer used.
Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 21, 2021

Choose a reason for hiding this comment

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

I was thinking this should become a property that mirrors the editors factory_str but I don't think that is needed / allowed? (ie an instance of an editor can't set its factory's trait). We ultimately had previously just set self.format = factory.format in the init method anyways.

I am worried there may be a change in behavior if a user was to try to set format on an instance of a RangeEditor. e.g my_editor = RangeEditor(), my_editor.format = "something". Previously, because we had been using self.format throughout instead of string_value, so this would actually be used. Now, AFAICT, the best we can do is pass it as a kwarg to string_value if a user tries to set it on an instance, but that will no longer take priority if the factory has its trait set...

I don't think we want to change any behavior here (even if the current behavior is strange). I am unsure how to best move forward.

@rahulporuri any thoughts on this? I hope my explanation made some sense, if not we can discuss tomorrow at standup

Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 21, 2021

Choose a reason for hiding this comment

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

So initially I expected this Test to pass on master, but fail on this branch:

def test_format_behavior_change(self):
        # format trait has been deprecated in favor of format_str. However,
        # behavior should be unchanged.
        model = RangeModel()
        with self.assertWarns(DeprecationWarning):
            my_editor = RangeEditor(format="%s ...")
            my_editor.format = "%s"
            view = View(
                Item(
                    "float_value",
                    editor=my_editor
                )
            )
        tester = UITester()
        with tester.create_ui(model, dict(view=view)) as ui:
            float_value_field = tester.find_by_name(ui, "float_value")
            float_value_text = float_value_field.locate(Textbox())
            self.assertEqual(
                float_value_text.inspect(DisplayedText()), "0.1"
            )

but it passes on both. I then realized my_editor is always just an instance of the traitsui.editors.RangeEditor EditorFactory. The actual Editor instances are only created when e.g. the _get_simple_editor_class method is called. If I access the Editor directly, e.g. doing something like this:

def test_format_behavior_change(self):
       # format trait has been deprecated in favor of format_str. However,
       # behavior should be unchanged.
       model = RangeModel()
       view = View(
           Item(
               "float_value",
               editor=RangeEditor(format="%s ...")
           )
       )
       tester = UITester()
       with tester.create_ui(model, dict(view=view)) as ui:
           float_value_field = tester.find_by_name(ui, "float_value")
           float_value_field._target.format = "%s"
           float_value_text = float_value_field.locate(Textbox())
           self.assertEqual(
               float_value_text.inspect(DisplayedText()), "0.1"
           )

I expected it to pass on master and fail on this branch. However, this fails on both...
So, AFAICT, behavior is not changed by this PR.

Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 22, 2021

Choose a reason for hiding this comment

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

EDIT: see the recently added test

Copy link
Contributor

@rahulporuri rahulporuri Jun 28, 2021

Choose a reason for hiding this comment

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

Can you search for users of the format trait on this editor? I'm not even sure how one would go about specifically searching for such uses. Whether or not users exist would help us decide on whether or not we need to convert format into a property as well - and make getting/setting format raise warnings as well.

Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 28, 2021

Choose a reason for hiding this comment

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

I was unable to find any users trying to set format on the Editor instance directly.
I did see users setting it on the Factory object, ie RangeEditor(format= ...), but that is covered by the use of property/deprecation in traitsui/editors/range_editor.py.

TBH I am not certain what the pattern would be to set it on the Editor instance. I guess accessing it through UIInfo?

Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 28, 2021

Choose a reason for hiding this comment

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

From traitsUI docs: "Note that not every trait on every editor can react to changes: some values are only used at editor creation time; however all editors support dynamically changing the enabled, visible and invalid traits. This feature can sometimes allow developers to avoid having to create a custom Handler subclass."

format = Str()

def init(self, parent):
Expand All @@ -94,8 +94,6 @@ def init(self, parent):
if not factory.high_name:
self.high = factory.high

self.format = factory.format

self.evaluate = factory.evaluate
self.sync_value(factory.evaluate_name, "evaluate", "from")

Expand All @@ -111,7 +109,7 @@ def init(self, parent):
try:
if not (self.low <= fvalue <= self.high):
fvalue = self.low
fvalue_text = self.format % fvalue
fvalue_text = self.string_value(fvalue)
except:
fvalue_text = ""
fvalue = self.low
Expand Down Expand Up @@ -153,11 +151,11 @@ def init(self, parent):

low_label = factory.low_label
if factory.low_name != "":
low_label = self.format % self.low
low_label = self.string_value(self.low)

high_label = factory.high_label
if factory.high_name != "":
high_label = self.format % self.high
high_label = self.string_value(self.high)

self._label_lo.setText(low_label)
self._label_hi.setText(high_label)
Expand All @@ -171,7 +169,7 @@ def update_object_on_scroll(self, pos):
""" Handles the user changing the current slider value.
"""
value = self._convert_from_slider(pos)
self.control.text.setText(self.format % value)
self.control.text.setText(self.string_value(value))
try:
self.value = value
except Exception as exc:
Expand Down Expand Up @@ -221,7 +219,7 @@ def update_editor(self):
low = self.low
high = self.high
try:
text = self.format % value
text = self.string_value(value)
1 / (low <= value <= high)
except:
text = ""
Expand Down Expand Up @@ -250,7 +248,7 @@ def _low_changed(self, low):
self.value = int(low)

if self._label_lo is not None:
self._label_lo.setText(self.format % low)
self._label_lo.setText(self.string_value(low))
self.update_editor()

def _high_changed(self, high):
Expand All @@ -261,7 +259,7 @@ def _high_changed(self, high):
self.value = int(high)

if self._label_hi is not None:
self._label_hi.setText(self.format % high)
self._label_hi.setText(self.string_value(high))
self.update_editor()

def _convert_to_slider(self, value):
Expand Down
74 changes: 74 additions & 0 deletions traitsui/tests/editors/test_range_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,3 +320,77 @@ def test_modify_slider_log_range_slider(self):
displayed = text.inspect(DisplayedText())
self.assertEqual(model.float_value, 10.0)
self.assertEqual(displayed, str(model.float_value))

def test_format_func(self):

def num_to_time(num):
minutes = int(num / 60)
if minutes < 10:
minutes_str = '0' + str(minutes)
else:
minutes_str = str(minutes)
seconds = num % 60
if seconds < 10:
seconds_str = '0' + str(seconds)
else:
seconds_str = str(seconds)
return minutes_str + ':' + seconds_str

model = RangeModel()
view = View(
Item(
"float_value",
editor=RangeEditor(format_func=num_to_time)
)
)
tester = UITester()
with tester.create_ui(model, dict(view=view)) as ui:
float_value_field = tester.find_by_name(ui, "float_value")
float_value_text = float_value_field.locate(Textbox())
self.assertEqual(
float_value_text.inspect(DisplayedText()), "00:00.1"
)

def test_editor_factory_format(self):
"""
format trait on RangeEditor editor factory has been deprecated in
favor of format_str. However, behavior should be unchanged.
"""
model = RangeModel()
with self.assertWarns(DeprecationWarning):
view = View(
Item(
"float_value",
editor=RangeEditor(format="%s ...")
)
)
tester = UITester()
with tester.create_ui(model, dict(view=view)) as ui:
float_value_field = tester.find_by_name(ui, "float_value")
float_value_text = float_value_field.locate(Textbox())
self.assertEqual(
float_value_text.inspect(DisplayedText()), "0.1 ..."
)

def test_editor_format(self):
"""
The format trait on an Editor instance previously potentially
could override the factory. Now that is not the case.
"""
model = RangeModel()
with self.assertWarns(DeprecationWarning):
view = View(
Item(
"float_value",
editor=RangeEditor(format="%s ...")
)
)
tester = UITester()
with tester.create_ui(model, dict(view=view)) as ui:
float_value_field = tester.find_by_name(ui, "float_value")
float_value_field._target.format = "%s +++"
model.float_value = 0.2
float_value_text = float_value_field.locate(Textbox())
self.assertEqual(
float_value_text.inspect(DisplayedText()), "0.2 ..."
)
18 changes: 8 additions & 10 deletions traitsui/wx/range_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class SimpleSliderEditor(BaseRangeEditor):
#: High value for the slider range
high = Any()

#: Formatting string used to format value and labels
#: Deprecated: This trait is no longer used.
Copy link
Contributor Author

@aaronayres35 aaronayres35 Jun 22, 2021

Choose a reason for hiding this comment

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

Rather than simply commenting on the deprecation, I would like to make this a property which raises a deprecation warning.
Ideally, getting the property would return the self.factory.format_str trait, and like wise setting it would set that trait. However it feels incorrect for an editor instance to be trying to change a trait on its factory...

Also as I think more about this PR, I am worried about Hyrum's law here. Even if the behavior change seems "more" correct, it is possible that users are dependent on the current behavior.

format = Str()

#: Flag indicating that the UI is in the process of being updated
Expand All @@ -91,8 +91,6 @@ def init(self, parent):
if not factory.high_name:
self.high = factory.high

self.format = factory.format

self.evaluate = factory.evaluate
self.sync_value(factory.evaluate_name, "evaluate", "from")

Expand All @@ -111,7 +109,7 @@ def init(self, parent):
fvalue = self.low
else:
try:
fvalue_text = self.format % fvalue
fvalue_text = self.string_value(fvalue)
except (ValueError, TypeError) as e:
fvalue_text = ""

Expand Down Expand Up @@ -157,11 +155,11 @@ def init(self, parent):

low_label = factory.low_label
if factory.low_name != "":
low_label = self.format % self.low
low_label = self.string_value(self.low)

high_label = factory.high_label
if factory.high_name != "":
high_label = self.format % self.high
high_label = self.string_value(self.high)

self._label_lo.SetLabel(low_label)
self._label_hi.SetLabel(high_label)
Expand Down Expand Up @@ -191,7 +189,7 @@ def update_object_on_scroll(self, event):
):
try:
self.ui_changing = True
self.control.text.SetValue(self.format % value)
self.control.text.SetValue(self.string_value(value))
self.value = value
except TraitError:
pass
Expand Down Expand Up @@ -253,7 +251,7 @@ def update_editor(self):
"""
value = self.value
try:
text = self.format % value
text = self.string_value(value)
1 // (self.low <= value <= self.high)
except:
text = ""
Expand Down Expand Up @@ -296,7 +294,7 @@ def _low_changed(self, low):
self.value = int(low)

if self._label_lo is not None:
self._label_lo.SetLabel(self.format % low)
self._label_lo.SetLabel(self.string_value(low))
self.update_editor()

def _high_changed(self, high):
Expand All @@ -307,7 +305,7 @@ def _high_changed(self, high):
self.value = int(high)

if self._label_hi is not None:
self._label_hi.SetLabel(self.format % high)
self._label_hi.SetLabel(self.string_value(high))
self.update_editor()


Expand Down