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.

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

Promote tools/compose to xkbcli-compile-compose #402

Merged
merged 4 commits into from
Nov 19, 2023

Conversation

wismill
Copy link
Member

@wismill wismill commented Nov 7, 2023

Fixes #381

@wismill wismill added enhancement Indicates new feature requests compose Indicates a need for improvements or additions to Compose handling cli labels Nov 7, 2023
@wismill wismill added this to the 1.7.0 milestone Nov 7, 2023
@wismill wismill requested review from bluetech and whot November 7, 2023 12:19
Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

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

nitpicks, overall LGTM, thanks. @bluetech may have a comment on why the locale handling was implemented like that first though.

tools/compile-compose.c Outdated Show resolved Hide resolved
tools/compile-compose.c Show resolved Hide resolved
tools/compile-compose.c Outdated Show resolved Hide resolved
tools/compile-compose.c Outdated Show resolved Hide resolved
tools/xkbcli-bash-completion.sh Show resolved Hide resolved
meson.build Show resolved Hide resolved
Copy link
Member Author

@wismill wismill left a comment

Choose a reason for hiding this comment

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

@whot thanks for the review. I fixed the code, but I am not sure where to put the escape utility function.

meson.build Show resolved Hide resolved
tools/compile-compose.c Outdated Show resolved Hide resolved
tools/compile-compose.c Outdated Show resolved Hide resolved
tools/xkbcli-bash-completion.sh Show resolved Hide resolved
tools/compile-compose.c Outdated Show resolved Hide resolved
tools/compile-compose.c Show resolved Hide resolved
tools/compile-compose.c Show resolved Hide resolved
@wismill wismill requested a review from whot November 9, 2023 11:51
@whot
Copy link
Contributor

whot commented Nov 10, 2023

I am not sure where to put the escape utility function.

based on the filename src/utf8.c seems like a good match? :)

@wismill
Copy link
Member Author

wismill commented Nov 12, 2023

I realized I did not push my changes…

based on the filename src/utf8.c seems like a good match? :)

@whot I would prefer to keep src/utf8.c only related to low-level encoding. So I chose src/compose/dump.h. Note that it could be adapted to be used in other contexts: it would need a flag to indicate whether to use hexadecimal escapes (compose-only) or octal ones.

Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

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

sorry, got no focus today so only two comments but I trust you enough that this will be fine.

src/compose/dump.h Outdated Show resolved Hide resolved
src/compose/dump.h Outdated Show resolved Hide resolved
test/compose.c Show resolved Hide resolved
test/compose.c Outdated
}
assert(buf[c] == '\0');
assert(strlen(buf) == c);
assert(is_valid_utf8(buf, c));
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this series: we should have something like

#define assert_printf(cond, ...) \
   if (!(cond)) { \
      printf("Assertion failure: __VA_ARGS__); \
      assert(cond); \
   }

to aid with debugging assertion failures.

Copy link
Member Author

Choose a reason for hiding this comment

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

I introduced assert_printf as you suggested, as well as assert_streq_not_null. I use them only for the new tests.

Copy link
Member Author

@wismill wismill left a comment

Choose a reason for hiding this comment

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

Fixed reviews comments and added a few comments.

@bluetech could you check my refactoring of locale handling?

src/compose/dump.h Outdated Show resolved Hide resolved
src/compose/dump.h Outdated Show resolved Hide resolved
test/compose.c Show resolved Hide resolved
test/compose.c Outdated
}
assert(buf[c] == '\0');
assert(strlen(buf) == c);
assert(is_valid_utf8(buf, c));
Copy link
Member Author

Choose a reason for hiding this comment

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

I introduced assert_printf as you suggested, as well as assert_streq_not_null. I use them only for the new tests.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Nice, it would be nice to have xkbcli compile-compose.

I left a few minor comments, but LGTM (feel free to merge after fixing).

src/compose/dump.h Outdated Show resolved Hide resolved
src/compose/dump.h Outdated Show resolved Hide resolved
src/compose/parser.c Outdated Show resolved Hide resolved
tools/compile-compose.c Show resolved Hide resolved
tools/compile-compose.c Outdated Show resolved Hide resolved
Previously this tool was only used for internal testing and thus
not installed. But it is useful for debugging, much like
xkbcli-compile-keymap.
`compgen` expect command options list formatted as a newline-separated
list. Add a missing newline when concatenating two lists.
Currently the result string is not escaped and may produce invalid
results.

Fixed by introducing an ad-hoc escape function and relative tests.
Current options to set the locale are convoluted:
- An explicit locale *must* be given, while a sane default would be
  to use the user environment.
- Then there are two options that were useful while testing locale
  handling: read environment variables or use `setlocale`. But the
  program has already called:
  ```
  setlocale(LC_ALL, "");
  ```
  so it turns out the two options lead to the same results.

Remove options `--locale-from-env` and `--locale-from-setlocale`
and make the locale default to the user environment.
@wismill wismill merged commit cfcc792 into xkbcommon:master Nov 19, 2023
4 checks passed
@wismill wismill deleted the compose/xkbcli branch November 19, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli compose Indicates a need for improvements or additions to Compose handling enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make tools/compose part of the xkbcli commands
3 participants