[haiku-commits] Re: r43210 - haiku/trunk/src/preferences/keymap
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Mon, 07 Nov 2011 16:21:02 +0100
On 07.11.2011 14:49, jscipione@xxxxxxxxx wrote:
Log:
Adds a dialog to the Keymap preference app to more easily and
reliably change your modifier keys.
Sounds like a good additional option.
Just pointing out a few minor coding style issues:
KeymapApplication::KeymapApplication()
- : BApplication("application/x-vnd.Haiku-Keymap")
+ : BApplication("application/x-vnd.Haiku-Keymap"),
+ fModifierKeysWindow(NULL)
Correct would be:
KeymapApplication::KeymapApplication()
:
BApplication("application/x-vnd.Haiku-Keymap"),
ModifierKeysWindow(NULL)
You actually did that in the source file you added yourself, though.
+++ haiku/trunk/src/preferences/keymap/KeymapApplication.h 2011-11-07
13:49:56 UTC (rev 43210)
@@ -12,6 +12,7 @@
#include "KeymapWindow.h"
+#include "ModifierKeysWindow.h"
#include<Application.h>
Just noting as you extended a previous violation: the local headers are
supposed to come last, unfortunately.
+ case kMsgUpdateModifiers:
+ {
+ uint32 keycode;
+
+ if (message->FindUInt32("caps_key",&keycode) == B_OK)
+ fCurrentMap.Map().caps_key = keycode;
+
+ if (message->FindUInt32("left_control_key",&keycode) ==
B_OK)
+ fCurrentMap.Map().left_control_key = keycode;
+
+ if (message->FindUInt32("right_control_key",&keycode)
== B_OK)
+ fCurrentMap.Map().right_control_key = keycode;
+
+ if (message->FindUInt32("left_option_key",&keycode) ==
B_OK)
+ fCurrentMap.Map().left_option_key = keycode;
+
+ if (message->FindUInt32("right_option_key",&keycode) ==
B_OK)
+ fCurrentMap.Map().right_option_key = keycode;
+
+ if (message->FindUInt32("left_command_key",&keycode) ==
B_OK)
+ fCurrentMap.Map().left_command_key = keycode;
+
+ if (message->FindUInt32("right_command_key",&keycode)
== B_OK)
+ fCurrentMap.Map().right_command_key = keycode;
It's probably Thunderbird again, but I take there is a space after the
comma?
Besides that, isn't there a simpler way to do this? Why not simply send
the updated keymap?
+++ haiku/trunk/src/preferences/keymap/ModifierKeysWindow.cpp 2011-11-07
13:49:56 UTC (rev 43210)
@@ -0,0 +1,496 @@
+/*
+ * Copyright 2011 Haiku Inc. All rights reserved.
+ * Distributed under the terms of the MIT License.
+ *
+ * Authors:
+ * John Scipione, jscipione@xxxxxxxxx
+ */
+
+#include "ModifierKeysWindow.h"
Two blank lines.
[...]
+#include "KeymapApplication.h"
+
+enum {
Two blank lines.
+static const uint32 kMsgCapsLockCapsLock = 'clcl';
+static const uint32 kMsgCapsLockControl = 'clct';
+static const uint32 kMsgCapsLockOption = 'clop';
+static const uint32 kMsgCapsLockCommand = 'clcm';
+static const uint32 kMsgCapsLockDisabled = 'clds';
[...]
+void
+ModifierKeysWindow::MessageReceived(BMessage* message)
+{
+ switch (message->what) {
+ // Caps Lock
+ case kMsgCapsLockCapsLock:
+ fCurrentMap->caps_key = 0x3b;
+
fCapsLockMenu->ItemAt(MENU_ITEM_CAPS_LOCK)->SetMarked(true);
+ _EnableRevertButton();
+ break;
[...]
When something looks like a repetitive tedious work, there is usually
something one can do about it.
Even if you might consider it pointless after all that work you already
put into it, clean code is always beneficial :-)
For this, I would have made a table like this (in pseudo code):
{location/type (ie. caps_key), map flags (ie. KEYS | DISABLE)}
{...}
foreach (key) {
foreach (map flag) {
message = new BMessage(kMsgUpdateModifier);
message->Add(...);
item = new BMenuItem(_KeyToString(key, flag), message);
if (fCurrentMap->key == key)
item->SetMarked(true);
popup->AddItem(item);
}
Which would simplify all sorts of code, and make this class much more
easier to read and maintain.
+ // Revert button
+ case kMsgRevertModifiers:
+ fCurrentMap->caps_key = fSavedMap->caps_key;
+ fCurrentMap->left_control_key =
fSavedMap->left_control_key;
+ fCurrentMap->right_control_key =
fSavedMap->right_control_key;
+ fCurrentMap->left_option_key =
fSavedMap->left_option_key;
+ fCurrentMap->right_option_key =
fSavedMap->right_option_key;
+ fCurrentMap->left_command_key =
fSavedMap->left_command_key;
+ fCurrentMap->right_command_key =
fSavedMap->right_command_key;
Since this window only ever changes the special keys, why not just do this:
memcpy(fCurrentMap, fSavedMap, sizeof(keymap));
+void
+ModifierKeysWindow::_EnableRevertButton()
+{
+ if (fCurrentMap->caps_key != fSavedMap->caps_key
+ || fCurrentMap->left_control_key != fSavedMap->left_control_key
+ || fCurrentMap->right_control_key !=
fSavedMap->right_control_key
+ || fCurrentMap->left_option_key != fSavedMap->left_option_key
+ || fCurrentMap->right_option_key != fSavedMap->right_option_key
+ || fCurrentMap->left_command_key != fSavedMap->left_command_key
+ || fCurrentMap->right_command_key !=
fSavedMap->right_command_key)
+ fRevertButton->SetEnabled(true);
+ else
+ fRevertButton->SetEnabled(false);
+}
Same here, why not just:
fReverButton->SetEnabled(memcmp(fCurrentMap, fSavedMap, sizeof(keymap))
!= 0);
+++ haiku/trunk/src/preferences/keymap/ModifierKeysWindow.h 2011-11-07
13:49:56 UTC (rev 43210)
[...]
+ char* fSavedBuffer;
+};
+
+#endif // MODIFIER_KEYS_WINDOW_H
Two blank lines.
Bye,
Axel.
Other related posts:
