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

Added a keyboard shortcut for renaming the current tab. #148

Merged
merged 3 commits into from
Oct 13, 2016

Conversation

tmr232
Copy link
Contributor

@tmr232 tmr232 commented Oct 9, 2016

Added the shortcut Ctrl+R to rename the current tab.
It also appears in the Window menu.

Closes #122. Relevant to #100 as well.

@takluyver
Copy link
Member

If we provide a way for the kernel to set the tab title as well (#147), how should they interact? Last one wins (as implemented now)? Or should the user manually setting it block changes coming from the kernel?

@tmr232
Copy link
Contributor Author

tmr232 commented Oct 9, 2016

I was just about to write in #147 that on second thought, I'd be happy with this PR as a solution to #100.
Letting the kernel change the title would require a lot of UX work to figure out properly.

@takluyver
Copy link
Member

If we don't do #147 for a while, I think this is OK, but I'll give @ccordoba12 a chance to have a look at it.

Copy link
Contributor

@wmvanvliet wmvanvliet left a comment

Choose a reason for hiding this comment

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

Some small nitpicks, otherwise LGTM.

old_title = self.tab_widget.tabText(self.tab_widget.currentIndex())
title, ok = QtGui.QInputDialog.getText(self,
"Rename Tab",
"New Title:".format(old_title),
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need the .format(old_title) here

Copy link
Contributor

Choose a reason for hiding this comment

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

New Title -> New title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think the label should read "New title:" and not "New Title", there is no reason to use title casing here.

@ccordoba12
Copy link
Collaborator

+1 from me :-)

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.

None yet

5 participants