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

Promote LEDEditor to a first-class editor #779

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

Conversation

corranwebster
Copy link
Contributor

It now has both Qt and Wx backends, so no reason not to.

It now has both Qt and Wx backends, so no reason not to.
@mdickinson
Copy link
Member

I notice that #778 removes the documentation for LEDEditor. Is that intentional, and should it be being readded here?

@mdickinson
Copy link
Member

I notice that #778 removes the documentation for LEDEditor

Rats. No it doesn't. It just moves it. Sorry for the noise.

@mdickinson
Copy link
Member

mdickinson commented Apr 3, 2020

In the demo, the fields are marked "left aligned", "right aligned", etc., but I don't see any difference in the alignments. Should I?

Screenshot 2020-04-03 at 11 44 13

(EDIT: this is with Python 3.8.2 / PyQt5 5.14.2 / macOS 10.14.6)

@mdickinson
Copy link
Member

Ah, I ran the demo before looking at the source, which says that alignment is wx-only.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

Ah, I ran the demo before looking at the source, which says that alignment is wx-only.

Hmm. The alignment doesn't seem to work with wx (wxPython4) either:

Screenshot 2020-04-03 at 11 53 22

# is also available online at http://www.enthought.com/licenses/BSD.txt
# Thanks for using Enthought open source!

""" A Traits UI editor that wraps a LED-style integer display.
Copy link
Member

Choose a reason for hiding this comment

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

typo: "a" -> "an"

# editor names for factory to find
ReadonlyEditor = LEDEditor
SimpleEditor = LEDEditor
CustomEditor = LEDEditor
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to populate all the three styles? If the custom/readonly editors are not implemented, then in the future we will be certain that they are never used because attempts to set style='custom' or style='readonly' will be met with an error (I think). If we populate them, it will add constraint to future extensions when we do want the custom / readonly style to behave completely differently.

e.g. The TableEditor does not have a custom editor. ButtonEditor does not have a readonly editor. They don't seem to be missed.

if ETSConfig.toolkit == 'wx':
from traitsui.wx.extra.led_editor import LEDEditor
else:
from traitsui.qt4.extra.led_editor import LEDEditor
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we keep traitsui.qt4.extra.led_editor and traitsui.wx.extra.led_editor as aliases? It costs almost nothing to maintain aliases. Otherwise this would be a breaking change.

.. automodule:: traitsui.editors.led_editor
:members:
:undoc-members:
:show-inheritance:
Copy link
Contributor

Choose a reason for hiding this comment

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

The API documentation is now being generated automatically. So this file and traitsui.editors.rst are not needed any more.

@@ -59,18 +53,12 @@ def update_editor(self):
"""
self.control.SetValue(self.str_value)

def _alignment_changed(self):
if self.control is not None:
self.control.SetAlignment(LEDStyles[self.alignment])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make such a change in a separate PR such that this PR is more focused about exposing the existing editor as first-class / built-in / whatever editor?

Furthermore I expect there being very limited use case to mutate the alignment after the editor is created - we can wait for this feature request to arise. The fewer mutables there are, the simpler and more maintainable the code is.

def init(self, parent):
""" Finishes initializing the editor by creating the underlying toolkit
widget.
"""
self.control = LEDNumberCtrl(parent, -1)
self.control.SetAlignment(LEDStyles[self.factory.alignment])
self.control.SetAlignment(LEDStyles[self.alignment])
Copy link
Contributor

@kitchoi kitchoi Jan 4, 2021

Choose a reason for hiding this comment

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

Same here, I think we can defer making alignment mutable in a separate PR / issue and restore the original code which uses the factory value.

(I bet one probably needs to sync_value method like this

self.sync_value(factory.adapter_name, "adapter", "from")
so that the changes are handled on the event loop and so that the value on the editor factory is actually transferred to the editor.) (edited: cross out this half of the comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I bet one probably needs to sync_value method like this...

Please ignore this half of the comment. The sync_value is used for synchronizing a trait on the editor with a trait on the object being edited. But the first half of the comment about defer making alignment mutable still stands.

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.

3 participants