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

citra-qt: Make hotkeys configurable via the GUI (Attempt 2) #4437

Merged
merged 7 commits into from Feb 4, 2019

Conversation

Projects
None yet
4 participants
@adityaruplaha
Copy link
Contributor

adityaruplaha commented Nov 16, 2018

This is take 2 at this. Take 1 is at #3786. All its commits have been merged into the first one to make rebase easier.
Tested on both MSVC & MinGW locally. All works & looks ok.

Original description is as follows.


Big thanks to @Kloen for letting me steal see & edit his code from Kloen/citra:hotkeys-once-again (originally #2773)!
EDIT: I've rewritten a lot of his code and have gone through the rest, so I can answer review questions regarding nearly all parts of it.

Pretty self explanatory. See the code for more details.
Note: This doesn't allow hotkeys to be bound to controllers.


Major changes:

  • Hotkeys separated into its own tab.
  • Registration code for hotkeys has been rewritten by @Kloen.
  • Config code for hotkeys has been almost completely rewritten.

Current Issues:

All fixed!! 🎉

Screenshots:

capture
For more, see the original PR.


This change is Reviewable

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Nov 16, 2018

Should I add the functionality of the missing hotkeys in this PR itself?

@zhaowenlan1779

This comment has been minimized.

Copy link
Member

zhaowenlan1779 commented Nov 17, 2018

please rebase

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Nov 17, 2018

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from 8dbbd33 to d58c6a3 Nov 17, 2018

@wwylele wwylele added canary-merge and removed canary-merge labels Nov 19, 2018

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from d58c6a3 to 1675fdb Nov 20, 2018

@wwylele wwylele added canary-merge and removed canary-merge labels Nov 20, 2018

@zhaowenlan1779

This comment has been minimized.

Copy link
Member

zhaowenlan1779 commented Dec 4, 2018

Please resolve conflicts and I think we can try tagging this again :p

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from 1675fdb to cac3c17 Dec 5, 2018

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from cac3c17 to c3803f4 Dec 5, 2018

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Dec 9, 2018

@Narugakuruga

This comment has been minimized.

Copy link

Narugakuruga commented Dec 13, 2018

Canary 1121 b8f2371 Windows 10
After changing graphics settings while game runs, hotkeys become unavailable. For example, F9 and F10 can't change screen layout, Ctrl+Z can't unlock speed limit.

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Dec 13, 2018

Show resolved Hide resolved src/citra_qt/hotkeys.cpp Outdated

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from e4fd8fe to 1820afb Dec 16, 2018

adityaruplaha added a commit to adityaruplaha/citra that referenced this pull request Dec 16, 2018

citra-qt: Use structured bindings where applicable in `ConfigureHotke…
…ys::applyConfiguration`. Fix a few minor issuesssssssss

* Address citra-emu#4437 (comment)

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from 1820afb to f042210 Dec 16, 2018

adityaruplaha added a commit to adityaruplaha/citra that referenced this pull request Dec 16, 2018

citra-qt: Use structured bindings where applicable in `ConfigureHotke…
…ys::applyConfiguration`. Fix a few minor issues.

* Address citra-emu#4437 (comment)
@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Dec 16, 2018

Fixed. Requesting a review from @jroweboy & @B3n30.

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch 2 times, most recently from 40bd55c to bc6bfff Jan 10, 2019

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Jan 12, 2019

Fixed that issue. Very big thanks to @wwylele for identifying the cause.

According to my tests, all works as expected.

  • Basic functionality
  • Config code
  • On-the-fly reloading

Please add pr:needs-review.

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 left a comment

Minor nits

@@ -48,6 +48,31 @@ const std::array<std::array<int, 5>, Settings::NativeAnalog::NumAnalogs> Config:
},
}};

// This shouldn't have anything except static initializers (no functions). So
// QKeySequnce(...).toString() is NOT ALLOWED HERE.

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Jan 17, 2019

Member

This two sentences don't look too necessary to me...

This comment has been minimized.

Copy link
@adityaruplaha

adityaruplaha Jan 19, 2019

Author Contributor

I thought I should've explained why can't it be used. It will cause a real bad problem if it's used. Especially if a debugger is unavailable.

Show resolved Hide resolved src/citra_qt/configuration/configure_input.cpp Outdated
Show resolved Hide resolved src/citra_qt/hotkeys.h Outdated
Show resolved Hide resolved src/citra_qt/hotkeys.h Outdated
Show resolved Hide resolved src/citra_qt/hotkeys.h Outdated
Show resolved Hide resolved src/citra_qt/hotkeys.cpp Outdated
@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Jan 18, 2019

Sorry, please resolve conflict again 😅

adityaruplaha added some commits May 22, 2018

citra-qt: Add base support for hotkey reconfiguration + UI (whole of PR
#3786)

* Adds a new Hotkeys tab in the Controls group.
* Right click to reconfigure.
* See the original PR for more details & screenshots.
citra-qt: Add back missing hotkeys & conflict fixes.
* Also fixed a missing spacer in ConfigureGeneral.
citra-qt: Use structured bindings where applicable in `ConfigureHotke…
…ys::applyConfiguration`. Fix a few minor issues.

* Address #4437 (comment)

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from ecd91cb to 4539e74 Jan 20, 2019

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 left a comment

Looks okay to me, but please fix the build (see comment 2).

Show resolved Hide resolved src/citra_qt/configuration/configure_hotkeys.cpp Outdated
Show resolved Hide resolved src/citra_qt/configuration/configure_input.h
Show resolved Hide resolved src/citra_qt/util/sequence_dialog/sequence_dialog.h Outdated
Show resolved Hide resolved src/citra_qt/configuration/configure_hotkeys.h Outdated
Show resolved Hide resolved src/citra_qt/configuration/configure_hotkeys.h Outdated

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from 4539e74 to 7cd6f21 Jan 24, 2019

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Jan 24, 2019

Sorry I didn't get much time to address review comments but I've fixed the build.

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from 65c0c92 to 7049cc8 Jan 25, 2019

@adityaruplaha adityaruplaha force-pushed the adityaruplaha:hotkey-config-squashed branch from 7049cc8 to 80ebd75 Jan 25, 2019

@adityaruplaha

This comment has been minimized.

Copy link
Contributor Author

adityaruplaha commented Jan 25, 2019

Fixed everything. Now is there any hope for it being merged?

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 left a comment

LGTM

@zhaowenlan1779 zhaowenlan1779 requested review from B3n30 and jroweboy Jan 29, 2019

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Feb 1, 2019

Will merge soon if no more comments

@wwylele wwylele merged commit f620c86 into citra-emu:master Feb 4, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/bitrise/4ccd8e5720f0d13b/pr Passed - citra
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@adityaruplaha adityaruplaha deleted the adityaruplaha:hotkey-config-squashed branch Feb 7, 2019

chris062689 pushed a commit to yuzu-emu/yuzu-nightly that referenced this pull request Apr 9, 2019

Merge pull request #2132 from FearlessTobi/port-4437
Port citra-emu/citra#4437: "citra-qt: Make hotkeys configurable via the GUI (Attempt 2)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.