[haiku-development] Re: Keymap management backend.
- From: Alexandre Deckner <alex@xxxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Sun, 07 Sep 2008 16:40:12 +0200
Michael Pfeiffer a écrit :
Am 05.09.2008 um 18:55 schrieb Alexandre Deckner:
Ok, here is a proof of concept, WIP, RFC, whatever you want to call
it. There's one important TODO left (see comment about keymap config
file), but the rest is pretty much done and working except the
ability to create new keymaps from a preflet.
So with this, you can:
- Change the keymap on a readonly system (think installer).
- Dramatically simplify the preflet code and make it safer, and
reliably show wich keymap is currently active.
- Easily write a keymap switcher.
- And maybe more...
For those interested, have a look at those two attached diffs, one
for the backend and one for the preflet.
I quickly skimmed through it and most parts look good as far as I can
tell.
Some comments:
* Maybe I applied to patches wrong, but it didn't work here.
I had to put Keymap[Manager].hpp into header/private/input and
had to create a static library with Keymap[Manager].hpp, that
is used by IS and Keymap preflet.
Hmm, don't know what went wrong here.
* You don't use new(std::no_throw) on purpose? Otherwise you have to
check for NULL return value.
* Use of stack allocated variables. In this case use a local variable
for example char str[5] instead:
+ default:
+ // 2-, 3-, or 4-byte UTF-8 character
+ {
+ char *str = new char[size + 1];
+ strncpy(str, &(chars[offset]), size);
+ str[size] = 0;
+ printf("%s", str);
+ delete [] str;
+ }
* Coding style:
* 80 chars per line is especially violated in switch blocks.
I would prefer one statement per line, but that's just me.
* Spaces between comparison operators i < 5 instead of i<5.
* This if expression looks strange to me:
bool
BKeymap::IsDeadSecondKey(uint32 keyCode, uint32 modifiers, uint8
activeDeadKey)
{
if (!activeDeadKey)
return false;
If activeDeadKey is a boolean its type should be bool,
otherwise the expression should be activeDeadKey == 0.
* No outer parentheses needed here: &(fChars[offset+1])
&fChars[offset+1] should be sufficient
* Possible buffer overflow in Keymap.cpp in method IsDeadKey:
char chars[4];
strncpy(chars, &(fChars[offset+1]), numBytes );
chars[numBytes] = 0;
According to a comment in print_key(...) numBytes can be max. 4.
Thanks, i'll have a look, but i forgot to mention i didn't write this,
that's a quick copy of src/preferences/keymap/Keymap.h/cpp .
* In KeymapWindow.cpp you're leaking BKeymap objects in SetKeymap.
Sorry, i didn't explicitly say i was aware of this, this is absolutely
not a final patch :) It's leaking in several places.
* You could use "using namespace BPrivate" so you don't have to prefix
everything from that name space.
Sure.
* In Keymap preflet: The Maps list loses focus after using cursor up
or down.
Yep, i noticed that too, although i think that's an old bug :)
* Feature request maybe for R2: Add support for different keyboard
layouts.
Agree.
Thanks a lot for reviewing!
Regards,
Alex
Other related posts:
