Simplify the preview of control mappings by removing the device path from controls #9700
Conversation
|
I honestly disagree with this PR. Knowing that it is not the default device is very important to the point that I'm ok with it being clunky. Plus if you are overriding the default device anyway you already have been in the advanced menu and know to look there. It's a feature for advanced users, so, it's ok. The original idea is hella clunky, oh yea, but I think we need to find a decent alternative rather than strip it out entirely. |
Unfortunately your argument is invalid, as whether the device path is in front of the control or not does not mean the device isn't the default one now, it just means that it wasn't the default one when you mapped it. Also, even if the above was true, what's the value in knowing to what device the first control of the expression belongs to? A few people complained about this on IRC and it's something I had also thought about in the past, so I decided to do it. This isn't perfect, and yeah it would be nice to also both the control and the device linked to a mapping, but I'd say this design overall outweighs the downsides of the current one. |
|
I agree that trying to display the full "path" in the little button is silly but it should be made clear that it has an explicit device specification. |
That does sound like a good compromise. Will do. |
|
Works for me |
|
@MayImilae @jordan-woyak Actually the best design would be to add a prefix number to the list of device [0], [1], ... and add that number to the preview of the mapping if that control has got a device path in front of it. Not sure I will do that, as the :A is already a decent solution. |
|
|
|
I'm kind of more in favor of the |
|
Just personally, the ":" seems like it's really hard to see. I was thinking what if we had some logic to ensure that the key was shown but then showed up to some maximum number of symbols. We could "..." the rest. Ex:
This could be used in other places in our GUI as well to denote longer strings than we have space for. |
Unfortunately I don't think that is any better than the current situation. So you can just enter anything into a controller slot and it'll just appear. It won't work, but you can see it. And when I put
It's the same for Dinput.
It would be exactly the same as configuring an alternate device in current master. It's just a very small space. This wouldn't achieve the desired goal of showing the input and not just the device. It may vary depending on the device and the text size, but fortunately we can all just try it. Just type in |
Hmm. Just to clarify my point. I was thinking we would dynamically determine how much text was viewable (or hardcode it) and then based on that remove from the device name as much as necessary. We'd always fit the key in. So if we had (you could tweak the algorithm if you always wanted a slash at the end) But this would at least show that we're not binding a simple key to the current device. It would also be more obvious than ":". |
|
Unfortunately the length isn't character based. Filling with |
|
Well, outside of a design overhaul, the only other thought would be some custom icon. But eh, I guess a ":" is fine. Certainly less work! |
Filoppi
force-pushed
the
simplify_mappings_preview
branch
from
4c8c4d4
to
f2d7ef9
Compare
|
@MayImilae done |
…their device path and replacing it with a ":" prefix. Also remove white spaces and \n \t \r. bugfix: fix EmulatedController::GetStateLock() not being aquired when reading the expression reference bugfix: MappingButton::UpdateIndicator() calling State(0) on outputs, breaking ongoing rumbles if a game was running Improvement: make expressions previews appear in Italic if they failed to parse correctly
Filoppi
force-pushed
the
simplify_mappings_preview
branch
from
f2d7ef9
to
0d23acc
Compare
|
Will this affect currently made controller profiles or configs? If it makes people have to reconfigure things it'd be annoying to them. Edit: I tested my profiles and they all seem to work so I guess not? |
No, this just affects the way Qt prints some text on screen, serialization isn't affected. |
|
I've asked a couple of times in IRC if anyone else has any qualms with this and haven't gotten any responses. This is an incredibly small change where I can mostly understand what the code is doing at a glance. |
PR dolphin-emu#9700 removed spaces from within control names, which some user complained about, and their point of view is kind of understandable: https://bugs.dolphin-emu.org/issues/12605 with this change, only spaces outside (between) control names are trimmed, which are the ones we wanted to trim in the first place. This will still retain the major advantages from 9700. Basically, "`Button 1` + `Button 2`" was showing as "`Button1`+`Button2`", while it will now show as "`Button 1`+`Button 2`".
The picture should explain everything. Simply, if the mapping did not use a default device when it was made, its preview was completely "useless".
Splitting by "`" is 100% safe as special characters have a distinct and unique meaning in controls expressions, so there couldn't be any false positives from this code.
The only downside is not seeing what device a control corresponds to, but you can finally see the control at least, which is still better than only seeing (part of) the device name. We now add the prefix ":" if a control has got a device specifier in front of them.
We also remove white spaces and \n \t \r from the mapping preview.
Extras:
bugfix: fix EmulatedController::GetStateLock() not being acquired when reading the expression reference
bugfix: MappingButton::UpdateIndicator() calling State(0) on outputs, breaking ongoing rumbles if a game was running.
Improvement: make expressions previews appear in Italic if they failed to parse correctly (highlighted in yellow in the screenshot) (expressions are still evaluated and become bold if their state is "true")