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

gui: remove unnecessary shortcuts in bitcoingui files #17128

Closed
wants to merge 1 commit into from
Closed

gui: remove unnecessary shortcuts in bitcoingui files #17128

wants to merge 1 commit into from

Conversation

GChuf
Copy link
Contributor

@GChuf GChuf commented Oct 13, 2019

This commit removes 2 shortcuts which are now unnecessary (see new shortcuts which were introduced in 091747b). Also removes some related unnecessary code.

@fanquake fanquake added the GUI label Oct 13, 2019
@hebasto
Copy link
Member

hebasto commented Oct 13, 2019

Concept ACK.

Indeed, Ctrl+Shift+C is replaced by Ctrl+T.
But the Ctrl+Shift+D shortcut is still quite useful, IMO.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 13, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@GChuf
Copy link
Contributor Author

GChuf commented Oct 13, 2019

@hebasto, I'll let other people know what they think about Ctrl+Shift+D - also, do you think it would be better to just use Ctrl+something and not use Shift?

@laanwj
Copy link
Member

laanwj commented Oct 14, 2019

FWIW: please try to use PR and commit titles that describe your change more precisely. "gui: clean bitcoingui files" doesn't convey much information.

@GChuf GChuf changed the title gui: clean bitcoingui files gui: remove unnecessary shortcut in bitcoingui files … Oct 15, 2019
@GChuf
Copy link
Contributor Author

GChuf commented Oct 15, 2019

Renamed, hopefully the title is clearer now.

@GChuf GChuf changed the title gui: remove unnecessary shortcut in bitcoingui files … gui: remove unnecessary shortcuts in bitcoingui files Oct 15, 2019
@laanwj
Copy link
Member

laanwj commented Oct 16, 2019

Renamed, hopefully the title is clearer now.

Yes, thanks!

@GChuf
Copy link
Contributor Author

GChuf commented Oct 30, 2019

Squashed and decided to keep the showDebugWindow shortcut as per hebasto's opinion.

Cleans unnecessary code after shortcuts were added in 091747b
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK f0fbb0d, tested on Linux Mint 19.2:

  • Ctrl+Shift+C - does nothing
  • Ctrl+{I|T|N|P} - behavior is unchanged

@luke-jr
Copy link
Member

luke-jr commented Nov 4, 2019

Concept NACK: Unless there's a reason to, we shouldn't break existing shortcuts...

@GChuf
Copy link
Contributor Author

GChuf commented Nov 4, 2019

@luke-jr the reasons are all explained if you would just read them

@luke-jr
Copy link
Member

luke-jr commented Nov 4, 2019

I see no reasons given. An additional shortcut key does not in any way justify removal of the existing one.

@jonasschnelli
Copy link
Contributor

Tend to NACK.
IMO there is no harm in keeping the "old" one (and I expect there are users using this shortcut heavily).

@jonasschnelli jonasschnelli removed their request for review November 4, 2019 19:46
@fanquake fanquake closed this Nov 4, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants