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.

Sign up for GitHub

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

Format unicode_common #13

Merged
merged 12 commits into from
Nov 3, 2018
+97 −114
Merged

Format unicode_common #13

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ae25652
case alignment
vomindoraan Nov 3, 2018
bcafc3b
process_record_unicode_common → process_unicode_common
vomindoraan Nov 3, 2018
e6d9692
Move song arrays into function where they're used, align preprocessor…
vomindoraan Nov 3, 2018
871169a
Swap the order of UC_WIN and UC_BSD
vomindoraan Nov 3, 2018
415d12a
Update Unicode docs
vomindoraan Nov 3, 2018
fdded5c
Reorder Unicode mode stuff to match the order of input mode constants
vomindoraan Nov 3, 2018
09dbc0c
Merge remote-tracking branch 'drashna/make_unicode_init' into format_…
vomindoraan Nov 3, 2018
3e22b6a
Fix capitalization in doc subtitle
vomindoraan Nov 3, 2018
2c9cfc4
Readd BSD and OSX_RALT songs
vomindoraan Nov 3, 2018
3f7cd36
Reword BSD note in docs
vomindoraan Nov 3, 2018
b55f059
Readd BSD keycode description
vomindoraan Nov 3, 2018
408ec5f
Reword explanation of input on different platforms
vomindoraan Nov 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 59 additions & 75 deletions quantum/process_keycode/process_unicode_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,6 @@

unicode_config_t unicode_config;
static uint8_t saved_mods;
#ifdef AUDIO_ENABLE
#ifdef UNICODE_SONG_OSX
float osx_song[][2] = UNICODE_SONG_OSX;
#endif
#ifdef UNICODE_SONG_LNX
float song_lnx[][2] = UNICODE_SONG_LNX;
#endif
#ifdef UNICODE_SONG_BSD
float song_bsd[][2] = UNICODE_SONG_BSD;
#endif
#ifdef UNICODE_SONG_WINDOWS
float windows_song[][2] = UNICODE_SONG_WINDOWS;
#endif
#ifdef UNICODE_SONG_WIN_COMPOSE
float win_compose_song[][2] = UNICODE_SONG_WIN_COMPOSE;
#endif
#ifdef UNICODE_SONG_OSX_RALT
float osx_ralt_song[][2] = UNICODE_SONG_OSX_RALT;
#endif
#endif


void set_unicode_input_mode(uint8_t os_target) {
unicode_config.input_mode = os_target;
Expand Down Expand Up @@ -90,16 +69,16 @@ void unicode_input_start(void) {
__attribute__((weak))
void unicode_input_finish(void) {
switch (unicode_config.input_mode) {
case UC_OSX:
case UC_WIN:
case UC_OSX:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is proper, but I hate it because of the readability (lack thereof)

Copy link
Author

@vomindoraan vomindoraan Nov 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Assuming you're referring to the case indentation) I actually disagree 😄 In QMK's case we have 2-space indents, but in projects where indentation is the standard 4 spaces or a tab, the extra indentation from cases (which bears no semantic meaning, as cases are just jump labels) can harm readability by adding another indentation level and shifting code horizontally.

To quote Linus Torvalds: “If you need more than 3 levels of indentation, you're screwed anyway, and should fix your program.”
And I'd rather not waste one of those levels on case labels 😄

But regardless of what we agree to go with, we should make sure we're consistent with it across all Unicode files: process_unicode_common.c, process_unicode.c, process_unicodemap.c, process_ucis.c.

case UC_WIN:
unregister_code(KC_LALT);
break;
case UC_OSX_RALT:
unregister_code(KC_RALT);
break;
case UC_LNX:
tap_code(KC_SPC);
break;
break;
case UC_OSX_RALT:
unregister_code(KC_RALT);
break;
case UC_LNX:
tap_code(KC_SPC);
break;
}

set_mods(saved_mods); // Reregister previously set mods
Expand Down Expand Up @@ -148,55 +127,60 @@ void send_unicode_hex_string(const char *str) {
}
}

bool process_record_unicode_common(uint16_t keycode, keyrecord_t *record) {
bool process_unicode_common(uint16_t keycode, keyrecord_t *record) {
if (record->event.pressed) {
switch (keycode) {
case UNICODE_MODE_OSX:
case UNICODE_MODE_OSX:
set_unicode_input_mode(UC_OSX);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_OSX)
PLAY_SONG(osx_song);
#endif
break;
case UNICODE_MODE_LNX:
set_unicode_input_mode(UC_LNX);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_LNX)
PLAY_SONG(song_lnx);
#endif
break;
case UNICODE_MODE_BSD:
set_unicode_input_mode(UC_BSD);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_BSD)
PLAY_SONG(song_bsd);
#endif
break;
case UNICODE_MODE_WIN:
set_unicode_input_mode(UC_WIN);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_WINDOWS)
PLAY_SONG(windows_song);
#endif
break;
case UNICODE_MODE_WINC:
set_unicode_input_mode(UC_WINC);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_WIN_COMPOSE)
PLAY_SONG(win_compose_song);
#endif
break;
case UNICODE_MODE_OSX_RALT:
set_unicode_input_mode(UC_OSX_RALT);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_OSX_RALT)
PLAY_SONG(osx_ralt_song);
#endif
break;
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_OSX)
Copy link
Author

@vomindoraan vomindoraan Nov 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I prefer preprocessor ifs (and preprocessor directives in general) to have their own separate indentation levels is that they aren't part of the program logic. They just pick & choose which parts of the code get compiled and which don't; they aren't runtime checks. Therefore, they shouldn't affect the indentation of the actual code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I definitely understand that. However, .... I think we need a clear and concise rule about this, because you can see three different formats throughout the code.

Also, this looks cleaner.

Copy link
Author

@vomindoraan vomindoraan Nov 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the tally for preprocessor directives before code in core QMK is:

  • 1209 matches across 154 files for directives at start of line (grepped for ^#.+\n +[^#]);
  • 651 matches across 45 files for indented directives (grepped for ^ +#.+\n +[^#]).

I agree that there should be a hard rule for it for any new code, as well as for the case indentation stuff (that's also done both ways in the codebase).

static float song_osx[][2] = UNICODE_SONG_OSX;
PLAY_SONG(song_osx);
#endif
break;
case UNICODE_MODE_LNX:
set_unicode_input_mode(UC_LNX);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_LNX)
static float song_lnx[][2] = UNICODE_SONG_LNX;
PLAY_SONG(song_lnx);
#endif
break;
case UNICODE_MODE_BSD:
set_unicode_input_mode(UC_BSD);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_BSD)
static float song_bsd[][2] = UNICODE_SONG_BSD;
PLAY_SONG(song_bsd);
#endif
break;
case UNICODE_MODE_WIN:
set_unicode_input_mode(UC_WIN);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_WIN)
static float song_win[][2] = UNICODE_SONG_WIN;
PLAY_SONG(song_win);
#endif
break;
case UNICODE_MODE_WINC:
set_unicode_input_mode(UC_WINC);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_WINC)
static float song_winc[][2] = UNICODE_SONG_WINC;
PLAY_SONG(song_winc);
#endif
break;
case UNICODE_MODE_OSX_RALT:
set_unicode_input_mode(UC_OSX_RALT);
#if defined(AUDIO_ENABLE) && defined(UNICODE_SONG_OSX_RALT)
static float song_osx_ralt[][2] = UNICODE_SONG_OSX_RALT;
PLAY_SONG(song_osx_ralt);
#endif
break;
}
}
#ifdef UNICODE_ENABLE
return process_unicode(keycode, record);
#endif
#ifdef UCIS_ENABLE
return process_ucis(keycode, record);
#endif
#ifdef UNICODEMAP_ENABLE
return process_unicode_map(keycode, record);
#endif
#if defined(UNICODE_ENABLE)
return process_unicode(keycode, record);
#elif defined(UNICODEMAP_ENABLE)
return process_unicode_map(keycode, record);
#elif defined(UCIS_ENABLE)
return process_ucis(keycode, record);
#else
return true;
#endif
}
10 changes: 5 additions & 5 deletions quantum/process_keycode/process_unicode_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
#include "quantum.h"

#if defined(UNICODE_ENABLE) + defined(UNICODEMAP_ENABLE) + defined(UCIS_ENABLE) > 1
#error "Cannot enable more than one unicode method (UNICODE, UNICODEMAP, UCIS) at the same time"
#error "Cannot enable more than one Unicode method (UNICODE, UNICODEMAP, UCIS) at the same time"
#endif

#ifndef UNICODE_TYPE_DELAY
#define UNICODE_TYPE_DELAY 10
#define UNICODE_TYPE_DELAY 10
#endif

typedef union {
Expand All @@ -42,12 +42,12 @@ void unicode_input_start(void);
void unicode_input_finish(void);
void register_hex(uint16_t hex);
void send_unicode_hex_string(const char *str);
bool process_record_unicode_common(uint16_t keycode, keyrecord_t *record);
bool process_unicode_common(uint16_t keycode, keyrecord_t *record);

#define UC_OSX 0 // Mac OS X
#define UC_LNX 1 // Linux
#define UC_WIN 2 // Windows 'HexNumpad'
#define UC_BSD 3 // BSD (not implemented)
#define UC_BSD 2 // BSD (not implemented)
#define UC_WIN 3 // Windows 'HexNumpad'
vomindoraan marked this conversation as resolved.
Show resolved Hide resolved
#define UC_WINC 4 // WinCompose https://github.com/samhocevar/wincompose
#define UC_OSX_RALT 5 // Mac OS X using Right Alt key for Unicode Compose

Expand Down
8 changes: 4 additions & 4 deletions quantum/quantum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ bool process_record_quantum(keyrecord_t *record) {
process_key_lock(&keycode, record) &&
#endif
#if defined(AUDIO_ENABLE) && defined(AUDIO_CLICKY)
process_clicky(keycode, record) &&
process_clicky(keycode, record) &&
#endif //AUDIO_CLICKY
process_record_kb(keycode, record) &&
#if defined(RGB_MATRIX_ENABLE) && defined(RGB_MATRIX_KEYPRESSES)
Expand All @@ -250,14 +250,14 @@ bool process_record_quantum(keyrecord_t *record) {
#ifdef STENO_ENABLE
process_steno(keycode, record) &&
#endif
#if ( defined(AUDIO_ENABLE) || (defined(MIDI_ENABLE) && defined(MIDI_BASIC))) && !defined(NO_MUSIC_MODE)
#if (defined(AUDIO_ENABLE) || (defined(MIDI_ENABLE) && defined(MIDI_BASIC))) && !defined(NO_MUSIC_MODE)
process_music(keycode, record) &&
#endif
#ifdef TAP_DANCE_ENABLE
process_tap_dance(keycode, record) &&
#endif
#if (defined(UNICODE_ENABLE) || defined(UNICODEMAP_ENABLE) || defined(UCIS_ENABLE))
process_record_unicode_common(keycode, record) &&
#if defined(UNICODE_ENABLE) || defined(UNICODEMAP_ENABLE) || defined(UCIS_ENABLE)
process_unicode_common(keycode, record) &&
#endif
#ifdef LEADER_ENABLE
process_leader(keycode, record) &&
Expand Down