Skip to content

[Trivial][GUI] Rename clear history and remove clsKey #1585

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.

Sign up for GitHub

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

Jump to bottom
Closed
Liquid369 wants to merge 1 commit into from
Closed

[Trivial][GUI] Rename clear history and remove clsKey #1585

Liquid369 wants to merge 1 commit into from

Conversation

@Liquid369
Copy link
Member

@Liquid369 Liquid369 commented Apr 30, 2020

Upon the creation of PR #1499 it needed some tidying up in my opinion.
Removal of the message at the top of console mentioning the usage of Ctrl + L to clear screen.
Removes related #ifdefs
Final outlook:
ClearHistory1

ambassador000
ambassador000 approved these changes May 11, 2020
View reviewed changes
Copy link

@ambassador000 ambassador000 left a comment

Choose a reason for hiding this comment

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

Functionality tested, looks better and more intuitive than current version.

@Fuzzbawls
Copy link
Collaborator

Fuzzbawls commented Jul 11, 2020

going to NACK this as I feel that a better solution would be to fix any issues with recognizing a keystroke to clear the history.

@Fuzzbawls Fuzzbawls mentioned this pull request Jul 11, 2020
Merged
@random-zebra
Copy link

random-zebra commented Jul 11, 2020

Agreed with @Fuzzbawls .
It would be better to restore the Ctrl + L functionality instead.
Also the "Clear History" label makes the button too wide in my opinion (and might not even fit for certain languages). For that kind of button, it would be better to have an icon (and maybe a tooltip).

@Liquid369
Copy link
Member Author

Liquid369 commented Jul 11, 2020 •
edited
Loading

I can make associated changes later, in hospital until Monday but I have github access for now obviously.

Are we keeping with old shortcut?
Ctrl + L?

Im personally more favorable of "Clear History" over a "-" for the button.
But really good point about checking translations. I will check those in the testing process depending what suggestions lead to my path for this work.

Although I am all for some sort of icon created, inserted and a tooltip added to say "Clear Screen/History"
I am just not a strong graphics person to create an icon.

random-zebra added a commit that referenced this pull request Jul 14, 2020
@random-zebra
Merge #1744: [GUI] Recognize key event for clearing console
fda84ee 
ff9e3d8 [GUI] Recognize key event for clearing console (Fuzzbawls)

Pull request description:

  Alternative to #1585 that leaves the wording unchanged, but adds key-combo recognition to the settingsconsolewidget the clear the history as how the initial help text describes.

  Adjust the settingsconsolewidget event filter to explicitly recognize
  the key combo for clearing the console.

  fixes #1213 which was actually not adequately resolved in #1499

ACKs for top commit:
  random-zebra:
    tested ACK ff9e3d8
  furszy:
    utACK ff9e3d8

Tree-SHA512: a7ac42fbab600334ba7ecd94474140aefbf1063172a8e97e0ee0235f7d75c6c98a6d0cca5d685891cc81cfa653fa1e86c2c45be0d502698cfcc19a62dfe69428
@Fuzzbawls
Copy link
Collaborator

Fuzzbawls commented Aug 2, 2020

superseded by #1744, closing

@Fuzzbawls Fuzzbawls closed this Aug 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ambassador000 ambassador000 ambassador000 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Liquid369 @Fuzzbawls @random-zebra @ambassador000