-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changed the IPython completion option from a checkbox to a combobox #2237
Conversation
I'm a complete amateur at Qt and don't know the spyder codebase at all so feel free to let me know if I've done things wrong. I'm hopeful that we'll get there in the end because I think the idea of letting the user have the full choice of completers is a good one. |
Thanks Dave! Long time no see :-) This seems quite reasonable. Right now I'm busy finishing 2.3.4, so I'll look at it next week. |
comp_group = QGroupBox(_("Completion Widget")) | ||
comp_label = QLabel(_("Decide what type of completion to use")) | ||
comp_label.setWordWrap(True) | ||
completers = [("plain", 0), ("droplist", 1), ("ncurses", 2)] |
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.
The default (as before) should be droplist
, not plain
. This is to match the same behavior as in the Editor and Python consoles (i.e. to use a completion widget :-)
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, I don't like the names used in completers
, given that those are shown in the dropdown box below too.
I know that these are the names used by IPython, but I think they are too technical (like how many people in the Windows world knows what ncurses
mean?).
What about these names?
- Graphical -> droplist
- Terminal -> ncurses
- Plain -> plain
I'm not sure about Terminal
but I can't come up with a better name :-)
I like this change, thanks for working on it. Other than my minor comments, I think this is ready to be merged :-) |
Sorry, been a bit busy. Thanks for the review - I'll try to address your comments today... |
8b316c2
to
b40cc44
Compare
b40cc44
to
e2bd6bb
Compare
completions = {True: 'droplist', False: 'ncurses'} | ||
spy_cfg.IPythonWidget.gui_completion = completions[gui_comp_o] | ||
completion_widget_o = CONF.get('ipython_console', 'completion_widget') | ||
completions = {0: "droplist", 1: "ncurses", 2: "plain"} |
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.
Note that these values remain the same as IPython ends up validating them against an Enum traitlet so the renaming only happens at the spyder/combobox level.
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.
Yes, there's no problem here because this is done internally. The problem is in the GUI, that's why I added my comment above :-)
Just another couple of minor changes, and we're ready to go :-) |
comp_label.setWordWrap(True) | ||
completers = [("Graphical", 0), ("Terminal", 1), ("Plain", 2)] | ||
comp_box = self.create_combobox(_("Completion:")+" ", completers, | ||
'completion_type', default=0) |
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.
Please remove the default
value. This is deprecated and it's going to be removed in 3.0.
Sorry I forgot to tell you before :-)
I'm going to merge this one and make the last suggested change myself, given that it's so simple :-) Thanks for your contribution :-) |
Changed the IPython completion option from a checkbox to a combobox
Thanks, for taking care of that. Sorry I didn't get to it - I've been busy moving countries so have been mostly offline for a while! |
I dislike both the
droplist
andncurses
completers. It seems to me to be an omission that there is no way (short of patching the source) to select theplain
tab completion.This PR changes the completion checkbox into a combobox which gives the user the option of all three possible completers.