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

Qt: Fix Hotkey Controller Profile display with boxes for each Wiimote #8654

Open
wants to merge 1 commit into
base: master
from

Conversation

@3t13nn3
Copy link

3t13nn3 commented Feb 27, 2020

According to https://bugs.dolphin-emu.org/issues/11646 the Hotkey Controller Profile GUI needed to have an overhaul.
Then @imStudd and I decided to solve this issue by putting theses options over boxes for each Wiimote. Separating Wiimotes looks way better and is more intuitive than displaying all Wiimotes options into one column.

Here is the old display:
Hotkeys Controller Profile

That is the new one:
Screenshot_2020-02-26_22-56-42

@3t13nn3 3t13nn3 changed the title Fix Hotkey Controller Profile display with boxes for each Wiimote Qt: Fix Hotkey Controller Profile display with boxes for each Wiimote Feb 27, 2020
@MayImilae

This comment has been minimized.

Copy link
Contributor

MayImilae commented Feb 27, 2020

Design wise, this is a nice little improvement!

0, 0);

m_main_layout->addWidget(
CreateGroupBox(tr("Wii Remote %2").arg(2),

This comment has been minimized.

Copy link
@jordan-woyak

jordan-woyak Feb 27, 2020

Member

Perhaps this was a find/replace mistake. %2 is probably meant to be %1.
Same problem below with 3 and 4.

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 27, 2020

Author

Then, I guess I have to switch 1 & 2, and 3 & 4 ?

This comment has been minimized.

Copy link
@jordan-woyak

jordan-woyak Feb 27, 2020

Member

Hmm? %1 is the format string placeholder for the first value supplied to the .arg function.

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 27, 2020

Author

Oh sure.
Edit: Done.

@@ -22,5 +22,5 @@ class HotkeyControllerProfile final : public MappingWidget
void CreateMainLayout();

// Main
QHBoxLayout* m_main_layout;
QGridLayout* m_main_layout;

This comment has been minimized.

Copy link
@jordan-woyak

jordan-woyak Feb 27, 2020

Member

It looks like this doesn't need to be a member variable as it's only used in CreateMainLayout.
You can just declare a local variable there and completely kill the above forward-declaration and include.
I realize you didn't add this member but now is a good time to fix it. :P

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 27, 2020

Author

OK, will change it to a local variable.
Edit: Done.

@3t13nn3 3t13nn3 force-pushed the 3t13nn3:HotkeyControllerProfile branch from 5ad4e83 to 4fbebd6 Feb 27, 2020
1, 0);

m_main_layout->addWidget(
CreateGroupBox(tr("Wii Remote %4").arg(4),

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Feb 27, 2020

Contributor

Are you sure this is the right way to format these? I'd assume the %1 is a placeholder for the argument and then the .arg() fills in the argument, so all of these should read "Wii Remote %1". Or just use literal strings and get rid of the .arg().

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 27, 2020

Author

As @jordan-woyak said upper, it must be 1% for the string format. I will commit it soon.
Edit: Done.

@3t13nn3 3t13nn3 force-pushed the 3t13nn3:HotkeyControllerProfile branch 2 times, most recently from 9fc0215 to 092349c Feb 27, 2020
@@ -6,7 +6,7 @@

#include "DolphinQt/Config/Mapping/MappingWidget.h"

class QHBoxLayout;
class QGridLayout;

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Feb 27, 2020

Contributor

You can remove this forward declaration now.

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 27, 2020

Author

Done.

QGridLayout* m_main_layout;
m_main_layout = new QGridLayout;
Comment on lines 19 to 20

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Feb 27, 2020

Contributor
Suggested change
QGridLayout* m_main_layout;
m_main_layout = new QGridLayout;
QGridLayout* m_main_layout = new QGridLayout;

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 27, 2020

Author

Done.

@3t13nn3 3t13nn3 force-pushed the 3t13nn3:HotkeyControllerProfile branch from 092349c to 9e936f1 Feb 27, 2020
_trans("Next Profile "),
_trans("Previous Profile "),
_trans("Next Game Profile "),
_trans("Previous Game Profile "),
Comment on lines 90 to 93

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Feb 27, 2020

Contributor

Just noticed one more thing, there's extra spaces at the end of these strings. You can even see that in your screenshot.

Maybe also clean up your commit message while you're at it, all those extra fix lines are really not necessary.

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 28, 2020

Author

Done.

@3t13nn3 3t13nn3 force-pushed the 3t13nn3:HotkeyControllerProfile branch from 9e936f1 to 95850fc Feb 28, 2020
QGridLayout* m_main_layout = new QGridLayout;

m_main_layout->addWidget(
CreateGroupBox(tr("Wii Remote %1").arg(1),

This comment has been minimized.

Copy link
@Techjar

Techjar Feb 28, 2020

Contributor

At this point using .arg() is superfluous, we should just put the number directly in the string since it's a compile-time constant. This would also allow translators to handle the numbers appropriately for their target language.

This comment has been minimized.

Copy link
@jordan-woyak

jordan-woyak Feb 28, 2020

Member

I can see the advantage of using the same string so it only has to be translated once.
I would be fine with either solution.


m_main_layout->addWidget(CreateGroupBox(
tr("Controller Profile"), HotkeyManagerEmu::GetHotkeyGroup(HKGP_CONTROLLER_PROFILE)));
QGridLayout* m_main_layout = new QGridLayout;

This comment has been minimized.

Copy link
@jordan-woyak

jordan-woyak Feb 28, 2020

Member

Please remove m_ from this variable name.
You can also declare the type as const auto.

This comment has been minimized.

Copy link
@3t13nn3

3t13nn3 Feb 28, 2020

Author

Done.

@3t13nn3 3t13nn3 force-pushed the 3t13nn3:HotkeyControllerProfile branch from 95850fc to 1303a85 Feb 28, 2020
@3t13nn3

This comment has been minimized.

Copy link
Author

3t13nn3 commented Feb 28, 2020

So actually, my teammate and I have discover that naming many options with same names poses a conflict problem. In fact, assigned value are mixed once we quit and restart Dolphin.
Furthermore, we found in ~/.config/dolphin-emu/Hotkeys.ini (the file that stock our inputs) that stocking depends on the name of the option.
So to avoid this, it seems we have to name each option differently.

1303a85#diff-8d3cfd109d3bb0eeb1cd0e968d40d086

We were using Next Profile as the next profile string but we need to change it. For that, we thought about:

  • return to Next Profile for Wii Remote 1
  • change to Next Profile 1
  • change to 1 Next Profile

Give us your opinion, so we can change.

If you find an other solution to avoid renaming each options differently, tell us !

Edit: As you can see bellow, no needs to renames option. We need to solve the conflict name issue.

@Techjar

This comment has been minimized.

Copy link
Contributor

Techjar commented Feb 28, 2020

The config key and the UI label being tied to the same string seems like a fundamental design flaw. Perhaps we should hold off on this change and fix that first?

@Techjar

This comment has been minimized.

Copy link
Contributor

Techjar commented Feb 29, 2020

#8655 should address the name conflict issue.

@3t13nn3

This comment has been minimized.

Copy link
Author

3t13nn3 commented Feb 29, 2020

Nice. We are waiting for you changes to be commited and we are done with this PR.

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Mar 1, 2020

Do we care about breaking existing configs?
This change will of course affect the names in the ini-file.

@Techjar

This comment has been minimized.

Copy link
Contributor

Techjar commented Mar 1, 2020

I don't think these hotkeys are that commonly used, and they don't have any default bindings, so in this case it's probably okay.

@Techjar

This comment has been minimized.

Copy link
Contributor

Techjar commented Mar 26, 2020

This needs to be rebased now that #8655 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.