a11y-keyboard: Add support for feedback when a key is pressed and CapsLock is active

[cwendling]

Adds support for beeping when a key is pressed while CapsLock is active.

This is useful for people with limited to no sight that might not remember or noticed CapsLock is active, and can prevent them from typing longish pieces of text before noticing it didn't type as expected.

This depends on https://github.com/mate-desktop/mate-desktop/pull/450. See also https://github.com/mate-desktop/mate-control-center/pull/583.

[cwendling]

Travis fails because of the mate-desktop dependency that isn't satisfied there.

[raveit65]

When mate-desktop PR is merged and mate-desktop-1.25.0 is released, we need to update build.yml of mate-settings-deamon. See https://github.com/mate-desktop/mate-settings-daemon/blob/master/.build.yml#L145

- cd ${START_DIR}
  - '[ -f mate-desktop-1.23.2.tar.xz ] || curl -Ls -o mate-desktop-1.23.2.tar.xz http://pub.mate-desktop.org/releases/1.23/mate-desktop-1.23.2.tar.xz'
  - tar xf mate-desktop-1.23.2.tar.xz
  - cd mate-desktop-1.23.2
  - if [ ${DISTRO_NAME} == "debian" -o ${DISTRO_NAME} == "ubuntu" ];then
  -     ./configure --prefix=/usr --libdir=/usr/lib/x86_64-linux-gnu --libexecdir=/usr/lib/x86_64-linux-gnu
  - else
  -     ./configure --prefix=/usr
  - fi
  - make
  - make install

This needs to be updated to mate-desktop-1.25.0.tar.xz

[raveit65]

@cwendling mate-desktop-1.25 is released.

[cwendling]

@raveit65 thanks, apparently it works now :)

[sc0w]

new warnings in the logs:

msd-a11y-keyboard-atspi.c:84:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined]
#if DESTROYING_ATSPI_LISTENER_CRASHES
    ^
msd-a11y-keyboard-atspi.c:32:13: note: expanded from macro 'DESTROYING_ATSPI_LISTENER_CRASHES'
        (! (defined (HAVE_LIBATSPI_NOBUG22) && HAVE_LIBATSPI_NOBUG22))
            ^

msd-a11y-keyboard-atspi.c:133:7: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined]
#if ! DESTROYING_ATSPI_LISTENER_CRASHES
      ^
msd-a11y-keyboard-atspi.c:32:13: note: expanded from macro 'DESTROYING_ATSPI_LISTENER_CRASHES'
        (! (defined (HAVE_LIBATSPI_NOBUG22) && HAVE_LIBATSPI_NOBUG22))
            ^

msd-a11y-keyboard-atspi.c:152:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined]
#if DESTROYING_ATSPI_LISTENER_CRASHES
    ^
msd-a11y-keyboard-atspi.c:32:13: note: expanded from macro 'DESTROYING_ATSPI_LISTENER_CRASHES'
        (! (defined (HAVE_LIBATSPI_NOBUG22) && HAVE_LIBATSPI_NOBUG22))
            ^

msd-a11y-keyboard-manager.c:992:49: warning: unused parameter 'key' [-Wunused-parameter]
                        gchar                  *key,
                                                ^

[sc0w]

NOTE: we are seeing now warnings in the logs unrelated to mate, warnings in the library atspi, for example:

/usr/include/at-spi-2.0/atspi/atspi-collection.h:43:1: warning: function declaration isn't a prototype [-Wstrict-prototypes]
   43 | GType atspi_collection_get_type ();
      | ^~~~~

@mate-desktop/core-team do you know how to silence the warnings of libraries at travis?

[rbuj]

I don’t think it’s necessary to do this, as the warnings are sometimes due to other parts of code, such as the expansion of CLAMP macro and unsigned integers.

[cwendling]

new warnings in the logs:

msd-a11y-keyboard-atspi.c:84:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined]
[…]

Oopsie, that's what I get for trying to get this extra thing in a little too fast (as the upstream bug wasn't fixed when I wrote the bulk of the patch, I didn't have a check for it set up apart the define hard-coded)… anyway, should be fixed now, I switched to a plain conditional definition.

msd-a11y-keyboard-manager.c:992:49: warning: unused parameter 'key' [-Wunused-parameter]

Indeed. I fixed this by marking the parameter as unused, as it's perfectly legitimate as it's a callback and this parameter is not useful in this case.

@mate-desktop/core-team do you know how to silence the warnings of libraries at travis?

AFAIK you can't, short of getting the headers of those libraries warnings-free. I tried to deal with this a while back, and I couldn't find a proper solution. The only remotely close one I encountered was trying to get the compiler recognize the header as "system" ones, but that's actually a lot different than just "installed" headers vs local ones, and IIRC had practical drawbacks.

[rbuj]

off-topic: @cwendling can you help us to review https://github.com/mate-desktop/mate-applets/pull/483?

[rbuj]

@mate-desktop/core-team

It cannot be merged until the team makes a decision see https://github.com/mate-desktop/mate-settings-daemon/pull/330#discussion_r467255944

[sc0w]

@rbuj I think we can wait one week and count the votes

[rbuj]

@sc0w I don’t know, I’ve waited longer for your reviews. I think the average is about a month.

[rbuj]

off-topic: We should write the decisions taken by the team somewhere, so those members who disagree, they recurrently ask to change the criterion, like using G_GNUC_UNUSED keyword, indents in the code formatting, declaration and assignment at the same line, among others we already talked about.

[cwendling]

off-topic: @cwendling can you help us to review mate-desktop/mate-applets#483?

I'd be happy to, but I might not have a setup to test this properly before at the very least a week, maybe 3. I'll try to remember to do it whenever I can.

[rbuj]

I think the vote can be left open, and it can be implemented later depending on the result, since it's a minor change and there are other requests that depend on this PR.

[raveit65]

Hmm, i am too late to party. Well, i don't really like to vote about a technical issue, a consensus after a discussion might be a better solution. Honestly, i have no idea if those warnings are really fault positives, another reason not to vote. IHMO, its a bit weird to use those warnflags to mate-compiler-flags.m4 from mate-common and disable them in code.

But every rule rule has an exception. If all you guys thinks that those warnings are fault positives, than it might be OK to disable them in code. We do the same for Gtk_Action deprecations warnings in a few packages, because it is impossible to fix them without removing icons from menus before we switch to gtk4.

But i really would like to see that author of PR and G_GNUC_UNUSED in code, file out an report at gtk to inform them about fault positive warnings and clarify the situation. If not i prefer not to disable them, maybe another developer is able to fix them?

[muktupavels]

If you want to build with -Wunused-parameter you sometimes have only two options, no? Use G_GNUC_UNUSED or trick compiler into thinking it is used ((void) parameter;)...

For example if you connect to signal that has 5 parameters (p1, p2, p3, p4, p5) and you only need to p1 and p5. You can not remove parameters from your callback function just because you don't need them.

I think it is perfectly fine to mark unused parameters to remove false positive warnings. Otherwise warnings just becomes spam in build log... And you will probably miss real/new warning between false positives.

For example capslock_beep_callback. It is connected to changed signal. Glib will always call it with 3 parameters - settings, key and user_data. In this example callback is connected specifying key name - capslock-beep-enable. So callback will be called only when capslock-beep-enable changes making key parameter uninteresting / unused. It is not glib "bug" because you don't need parameter, it is not code bug because you don't need key in this case. @rbuj, if you say it is not good idea to silence this warning in this case, what is good idea?

[rbuj]

It depends on the case, as this warning is closely linked to -Werror=cast-function-type.

[raveit65]

If you want to build with -Wunused-parameter you sometimes have only two options, no? Use G_GNUC_UNUSED or trick compiler into thinking it is used ((void) parameter;)... For example if you connect to signal that has 5 parameters (p1, p2, p3, p4, p5) and you only need to p1 and p5. You can not remove parameters from your callback function just because you don't need them. I think it is perfectly fine to mark unused parameters to remove false positive warnings. Otherwise warnings just becomes spam in build log... And you will probably miss real/new warning between false positives.

Ok, than i am fine to mark fault positives as unused parameters in code. Thx for clarification.

[sc0w]

I did PR with G_GNUC_UNUSED at some points: https://github.com/mate-desktop/mate-settings-daemon/pull/332

[raveit65]

off-topic: We should write the decisions taken by the team somewhere, so those members who disagree, they recurrently ask to change the criterion, like using G_GNUC_UNUSED keyword, indents in the code formatting, declaration and assignment at the same line, among others we already talked about.

Our Wiki?