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

Formal editor interface for tooltips #1493

Merged
merged 7 commits into from
Jan 22, 2021

Conversation

corranwebster
Copy link
Contributor

This is a refactor/cleanup that does a number of things:

  • makes the tooltip methods a formal part of the base Editor interface. Previously these were supplied by the base toolkit implementations of the Editor classes, so they were there but not documented.
  • moves out the logic for finding the tooltip text into its own method (which could be overridden if needed)
  • moves out the toolkit-specific code into its own abstract method and making set_tooltip a method on the base Editor class, removing all duplicated code
  • adds tests

There are strategic code-generation reasons why I want this done, but hopefully it is useful enough to stand as a clean-up action.

@rahulporuri rahulporuri added this to In progress in Enthought OSS Q1 2021 Jan 20, 2021
traitsui/editor.py Outdated Show resolved Hide resolved
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Mostly LGTM with a few comments/questions.

There are strategic code-generation reasons why I want this done, ...

Can you elaborate on what you mean by "strategic code-generation reasons"? I have no clue what you're referring to.

traitsui/editor.py Outdated Show resolved Hide resolved

Returns
-------
tooltip_set : bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont understand why set_tooltip returns a boolean and i'm not sure where/who users the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did check: it is used. Not sure what for, I didn't dig in.

@corranwebster
Copy link
Contributor Author

Can you elaborate on what you mean by "strategic code-generation reasons"? I have no clue what you're referring to.

"Code-generation" was bad phrasing: what I'm particularly interested in is that code like UIEditors, CodeEditor, and PythonShell which aren't simple wrappers around a toolkit control, but rather wrap TraitsUI or Pyface objects, we still have to have toolkit-specific classes (which in some cases like ArrayViewEditor simply inherit from the toolkit Editor and the common ArrayViewEditor with no other content). In these classes and for similar user-defined classes, I'd like it to be easy to have editors which don't need to have empty toolkit-specific subclasses.

So "strategically supporting new editor generation" is probably better. But as I said, this should be evaluated independently: there is an API that is implemented everywhere, we should raise it up and make it formal.

@corranwebster
Copy link
Contributor Author

I should add: at some point I may make a similar PR about sizing, as there is an interface there that should be made common and formal, but I haven't looked into it.

@corranwebster corranwebster merged commit 7f3dbaf into master Jan 22, 2021
Enthought OSS Q1 2021 automation moved this from In progress to Done Jan 22, 2021
@corranwebster corranwebster deleted the enh/editor-interface-tooltip branch January 22, 2021 17:38
@rahulporuri rahulporuri moved this from Done to Sprint 1 : Jan 4 2021 - Jan 22 2021 in Enthought OSS Q1 2021 Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Enthought OSS Q1 2021
Sprint 1 : Jan 4 2021 - Jan 22 2021
Development

Successfully merging this pull request may close these issues.

None yet

2 participants