Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

Push-to-talk using Libuiohook #6185

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

anthonybilinski
Copy link
Member

@anthonybilinski anthonybilinski commented Jul 7, 2020


This change is Reviewable

This is currently in a working state. TODOs are

  • Add multi-key support on scanning end. Settings/saving already work. This would involve tracking which of the keys are pressed.
  • Improve key-setting UI to display key names rather than their numbers.
  • Make passing of key list to hook callback threadsafe with some non-blocking locking, failing to lock is better than blocking to avoid hanging whole system.
  • Improve logging
  • Some code cleanup with inline TODOs
  • Dynamically destruct when PTT is not selected in AV settings, construct when it is
  • Add build for OSX and Windows, update build instructions for Linux
  • Remove singleton Settings from HotkeyInput. Either delay construction until AvForm and pass it in, or move functionality that requires Settings outside of class.
  • Clean up HotkeyInput (remove globals etc.)
  • Dynamically load Libuiohook since it depends on screen, causing unit tests to fail
  • Clean up git history

Overall this is way less work than the previous implementations. A couple limitations using this approach:

  1. On X11, shortcut keys are still passed through to the underlying application. All libs I saw either use XGrabKey which causes the application to briefly grab focus, eating all other key strokes that happen while the shortcut is held, or have this behaviour. The keys are blockable on OSX and Windows.
  2. We get tons of info, way beyond what we need, e.g. all mouse and keyboard input of any kind. This is a bit of a privacy concern, but is the nature of the OS APIs that are provided. De-registering when PTT is not selected in settings and having it off by default should help with that.
  3. Libuiohook has no void* in the callback, so we have no way to get back to our class, meaning our impl needs some gross global state. This isn't that bad since this is conceptually a singleton concept anyway.

@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 6 times, most recently from 0451681 to 492d1a3 Compare July 9, 2020 21:46
@anthonybilinski anthonybilinski changed the title WIP: Push-to-talk using Libuiohook Push-to-talk using Libuiohook Jul 9, 2020
@anthonybilinski
Copy link
Member Author

I think the code is ready to go, there's just the small issue of libuiohook causing every test to segfault in CI.

This is because it's doing registration with the system during library load, and that's failing because there's no display present. To work around this I'll have to avoid linking libuiohook until qTox can verify that a display is present, then load it in code at that time.

@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 3 times, most recently from b182617 to a9e1006 Compare July 14, 2020 10:57
@anthonybilinski
Copy link
Member Author

Updated to load libuiohook at runtime instead of linking to it, and made the AV form's option for PTT dynamically dependent on the ability to load the lib, and I've cleaned up git history to a point where I think this is ready to be reviewed and tested.

It looks like CI is failing due to this issue:

Resolving download.libsodium.org (download.libsodium.org)... 37.59.238.213
Connecting to download.libsodium.org (download.libsodium.org)|37.59.238.213|:443... connected.
GnuTLS: A TLS fatal alert has been received.
GnuTLS: received alert [70]: Error in protocol version
Unable to establish SSL connection.

which I've been seeing for about a week.

@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 2 times, most recently from 1e1c7db to 3a04c04 Compare July 18, 2020 06:32
@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 11 times, most recently from 66b7085 to f49181b Compare August 11, 2020 06:51
@anthonybilinski
Copy link
Member Author

Tested on Windows, a couple issues:

  1. The library is named libuiohook-0.dll so we fail to find it since we're looking for libuiohook.dll. On Linux it's called libuiohook.so so our name in code is just libuiohook. I don't know why the naming is different on the different systems, but at worst we can just rename the library during build.
  2. The key blocking works correctly (which isn't possible on X11), but due to the way we set shortcuts, they continue being blocked until a new shortcut is set. So e.g. it's impossible to directly change from "A+S+D" -> "A" since the "A" input will be blocked. To fix this we should clear the setting as soon as the box is selected, then re-set the old combo if no new combo is set.

Other than that it works well. OSX isn't tested yet, but I've got an environment set up to test, just don't see where I can grab the build from from the CI. Will build locally a try it, but at least issue 2) should still be present.

@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 2 times, most recently from 0ec1f30 to bd47313 Compare August 16, 2020 06:31
@anthonybilinski
Copy link
Member Author

I've added a single commit backing out the (otherwise working) macOS implementation. This commit can be backed out when #6214 is fixed. I'd still like to merge this PR as-is since setting up permission management is a fairly unrelated task, this works fully on Windows, and this would help testing on macOS in the future by just backing out the last commit.

If the reviewer doesn't want a change that includes then backs out macOS support that's reasonable, but I think it would be nice to have a finished working version of adding hook to the macOS for future use.

@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 3 times, most recently from 026f826 to 2e5fed5 Compare August 19, 2020 22:22
@anthonybilinski
Copy link
Member Author

On second thought adding macOS support then backing it out in the same PR is dumb. I've just added support on top of this change and pushed that to https://github.com/anthonybilinski/qTox/tree/libuiohook_ptt_macos, which I've mentioned in #6214. This PR now only adds Linux and Windows support, and doesn't touch macOS at all.

Once permissions are properly handled on macOS in #6214, it should be very easy to just include the one commit from https://github.com/anthonybilinski/qTox/tree/libuiohook_ptt_macos.

@anthonybilinski anthonybilinski force-pushed the libuiohook_ptt branch 2 times, most recently from 4583014 to e0f58fb Compare August 20, 2020 00:22
@sphaerophoria
Copy link
Contributor

sphaerophoria commented Aug 20, 2020

Trying it out and just a couple comments

  • It's pretty funny that this is printed in qtox logs on every boot
libUIOHook: Cross-platfrom userland keyboard and mouse hooking.
Copyright (C) 2006-2015 Alexander Barker.  All Rights Received.
https://github.com/kwhat/libuiohook/

libUIOHook is free software: you can redistribute it and/or modify
it under the terms of the GNU Lesser General Public License as published
by the Free Software Foundation, either version 3 of the License, or
(at your option) any later version.

libUIOHook is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU Lesser General Public License
along with this program.  If not, see <http://www.gnu.org/licenses/>.

This may be related to my libuiohook version. I have version 1.0.3 from https://aur.archlinux.org/packages/libuiohook/

  • I think there should be a visual indicator in the audio settings page of when the PTT key is active. This helps first time users understand how the feature works and test out different keys.
  • Right ctrl/left ctrl display the same in the preview but which ctrl key you have bound is important
  • If I press right alt I see
    image
  • This empty space is weird
    image
  • The lack of a way to clear the button is confusing. I agree it's unneeded because of the capture mode dropdown, but maybe writing that in the "Press any key combo" box would make it more clear
  • I couldn't bind to a mouse button
  • I'd prefer if there was a way to block the input keys from being passed through to other applications, but IIRC from conversations with @anthonybilinski this is not possible on linux due to X11 limitations. @anthonybilinski it would be nice to link the libuiohook issue here for future reference
  • My machine is behaving as push to mute instead of push to talk. I'm not sure if that's a mislabeling or I've done something wrong
  • The feature itself seems to work though 👍

I'm not sure if any of these should block the PR, just making them known just in case. @anthonybilinski let me know if you want me to proceed with review now or hold off until some of the above are addressed.

tox-user and others added 13 commits September 7, 2020 00:23
Adds a selection of audio capture mode (continuous/voice activation/push to
talk) in audio/video settings and an ability to set a push to talk hotkey.
We still need to also save the key codes though since the QKeyEvent holds both
but we can't necessarily convert from one to the other.
The mute option in each call is toggled when the key combo is pressed, so this
feature is also effectively push-to-mute as well if users are unmuted in chat
to start.

Load libuiohook at runtime because it registers with the OS on library load,
and segfaults if no window system is available, e.g. for unit tests.
Don't add to macOS build yet due to macOS specific issues, qTox#6214
To avoid the old shortcut from interfering with the entering of the new one.
Clearing all PTT keys just puts us in a state where the input mode is still
PTT, but we're effectively in continuous mode. Just let the user change to
continuous mode to disable PTT. Also the text said that esc cancelled, but
really leaving focus cancels and ESC just cleared.
@sudden6
Copy link
Member

sudden6 commented Sep 9, 2020

Installed this but seems libuiohook is not found by qTox.

Install log:

Install the project...
-- Install configuration: ""
-- Installing: /usr/local/lib64/libuiohook.so.1.2.0
-- Installing: /usr/local/lib64/libuiohook.so.1
-- Installing: /usr/local/lib64/libuiohook.so
-- Up-to-date: /usr/local/include/uiohook.h
-- Old export file "/usr/local/lib64/cmake/uiohook/uiohook-config.cmake" will be replaced.  Removing files [/usr/local/lib64/cmake/uiohook/uiohook-config-noconfig.cmake].
-- Installing: /usr/local/lib64/cmake/uiohook/uiohook-config.cmake
-- Installing: /usr/local/lib64/cmake/uiohook/uiohook-config-noconfig.cmake
-- Up-to-date: /usr/local/lib64/pkgconfig/uiohook.pc

No relevant output in qTox log. I think it could make sense to log if libuiohook couldn't be found, makes debugging easier.

I don't need any special build flags for qTox, do I?

UI for audio activation looks very nice now.

Copy link
Member

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 19 files at r6.
Reviewable status: 0 of 1 LGTMs obtained


src/hookmanager.cpp, line 43 at r6 (raw file):

        case UIOHOOK_SUCCESS:
            return;
        

whitespace error


src/persistence/settings.h, line 367 at r6 (raw file):

    }
    void setOutVolume(int volume) override;
    

whitespace error


src/persistence/settings.h, line 370 at r6 (raw file):

    AudioCaptureMode getAudioCaptureMode() const override;
    void setAudioCaptureMode(AudioCaptureMode mode) override;
    

whitespace error


src/persistence/settings.h, line 376 at r6 (raw file):

    QList<int> getPttShortcutNames() const override;
    void setPttShortcutNames(QList<int> keys) override;   
    

whitespace error


src/widget/form/settings/avform.cpp, line 581 at r6 (raw file):

    pushToTalkShortcutLabel->hide();
    pushToTalkShortcutInput->hide();
    

whitespace error


src/widget/form/settings/avform.cpp, line 598 at r6 (raw file):

void AVForm::on_inModeComboBox_currentIndexChanged(int index)
{
    const IAudioSettings::AudioCaptureMode mode = static_cast<IAudioSettings::AudioCaptureMode>(inModeComboBox->currentData().toInt()); 

whitespace error

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants