Opened 5 years ago

Closed 4 years ago

#9665 closed Feature Requests (Fixed)

Tidy up keypress handling

Reported by: jhsrennie Owned by: jhsrennie
Priority: 1 - Lowest Milestone: 11.0 "Eden"
Component: Keymapping (Remote Control / Gamepad Controller) Version: GIT
Severity: Minor Keywords:
Cc: jmarshall Blocked By:
Blocking: Platform: All


The current keypress handling is untidy because the g_Keyboard object is used in places like CApplication::OnKey. It would be tidier for all the information needed by OnKey to be passed in the CKey argument.

The changes in the patch extend the CKey object with extra member variables for e.g. the key modifier state. A new method CKeyboardStat::GetKey is added that returns a CKey with all the information needed to process the keystroke so no further reference to g_Keyboard need be made.

This is a first draft. It builds and works but has not been thoroughly tested. The aim of this ticket is to solicit views whether it is worth continuing down this route.

Attachments (4)

KeyHandling-firstdraft.patch (9.4 KB) - added by jhsrennie 5 years ago.
KeyHandling-100720.patch (14.6 KB) - added by jhsrennie 5 years ago.
KeyHandling-100721.patch (4.3 KB) - added by jhsrennie 5 years ago.
CKeyboardStat::GetKey now returns a const CKey
keyboard_cosmetics.patch (6.7 KB) - added by jmarshall 5 years ago.

Download all attachments as: .zip

Change History (16)

Changed 5 years ago by jhsrennie

comment:1 Changed 5 years ago by jmarshall

Nice work - this is definitely wanted.

I suggest feeding the key state into CKey via the constructor: After all, there should be no need to actually set the key codes or modifiers outside of the CKeyboard::GetKey() function.

I think I'd prefer the modifiers to use something like GetModifiers() but don't really have a preference there - whatever works best for the code that's using it should be the priority.

The buttoncode variable should be in the if block by the looks, and no need for g_Keyboard.GetHeld() there when you can just use the member directly.

comment:2 Changed 5 years ago by jhsrennie

Thanks, I've modified the constructor and moved the declaration of buttoncode (where have I used g_Keyboard.GetHeld()?) and committed the changes. I'd stick with GetCtrl, GetShift etc as they make the code very clear. They're inline functions so the overhead is minimal.

I've left large chunks of old code present but commented out in case I've broken something and need to go back. The next job is to tidy up the unholy mess that is CKeyboardStat::Update :-)

comment:3 Changed 5 years ago by jmarshall

Any reason why we can't push a bitmask into the constructor instead of all those bools - it just makes things messy IMO. Also, the Set*() functions aren't needed anymore, right?

I presume that everything is converted to a bitmask later for whatever happens in the action translator, or is that handled some other way?

comment:4 Changed 5 years ago by jhsrennie

I'm happy to use a bitmask. In fact CKey defines masks that are used by ButtonTranslator so the constants already exist. I'll have a look at this.

Re the SetXXX functions. At the moment CApplication::ProcessKeyboard declares a CKey and passes it to CKeyboardStat::GetKey to be initialised. That means the CKey member variables can't be set in the constructor. I could have GetKey declare a new CKey with everything in the constructor and copy it into the CKey passed by ProcessKeyboard, but is this any less tacky than having the SetXXX functions?

An alternative would be to have CKey declare CKeyboardStat as a friend and just use the member variables directly.

Changed 5 years ago by jhsrennie

comment:5 Changed 5 years ago by jhsrennie

See KeyHandling-100720.patch

Both CKey and CKeyboardStat now use a uint32_t bitmask to hold the key modifiers. This greatly simplifies the code. Added a CKey::SetKeystroke method to allow easy setting of the CKey member variables.

comment:6 Changed 5 years ago by jmarshall

Looks good, though the Getters for modifiers should probably use

return (m_Modifiers & MODIFIER_CTRL) == MODIFIER_CTRL;

1/0 is not necessarily the same as true/false (you'll likely get a compile warning).

The main advantage with no Setters is abstract - it signifies that the CKey object is always const - once constructed it cannot be changed.

Changed 5 years ago by jhsrennie

CKeyboardStat::GetKey now returns a const CKey

comment:7 Changed 5 years ago by jhsrennie

See KeyHandling-100721.patch

Make changes to Getters suggested by jmarshall. CKeyboardStat::GetKey now returns a const CKey. Remove the CKey::SetKeystroke function. There are still a few Setters e.g. SetFromHttpApi that I'm reluctant to touch for now as I've broken arnova's build once already!

Changed 5 years ago by jmarshall

comment:8 Changed 5 years ago by jmarshall

The attached patch is essentially cosmetic. IMO it cleans things up a bit to be more consistent with other areas/code style in general.

comment:9 Changed 5 years ago by jhsrennie

I have committed jmarshall's changes. If all interested parties (basically just me and jmarrshall I think) are happy with the changes I will close this ticket.

comment:10 Changed 5 years ago by jmarshall

Only other thing I see is that the copy constructor is unnecessary - all the members appear to be assignable using a default copy constructor?

comment:11 Changed 4 years ago by sho

  • Component changed from Other (un-categorized) to Keymapping (Remote Control / Gamepad Controller)

So are you happy?

comment:12 Changed 4 years ago by jhsrennie

  • Milestone changed from Future / Pending to 11.0
  • Resolution set to Fixed
  • Status changed from new to closed

All working and it's in the trunk for v11.

Note: See TracTickets for help on using tickets.