gh-obsproject/obs-browser
Watch
1
0
Fork
You've already forked obs-browser
 0
Code Issues 37 Pull requests 13 Releases 15 Wiki Activity

obs-browser: Fix key mapping on linux #206

Merged
Francexi merged 1 commit from master into master 2020-04-02 14:44:11 -05:00
Conversation 9 Commits 1 Files changed 3 +781 -3
Francexi commented 2020-03-05 11:32:44 -06:00 (Migrated from github.com)

Description

The fix maps, only for Linux systems, XKB keycodes (that are provided to the SendKeyClick function) to the Javascript ones. The fix also add the native_scancode as value for the native_vkey, that seems to make the fix work also on Windows. (thanks to WizardCM for the collab)

Motivation and Context

The keypress event was inusable on obs-browser. This seems to fully fix this

Fixes #200

How Has This Been Tested?

Tested on Slackware-current, using CEF build 3809. As long as I tested, all seems to work fine

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
### Description The fix maps, only for Linux systems, XKB keycodes (that are provided to the SendKeyClick function) to the Javascript ones. The fix also add the native_scancode as value for the native_vkey, that seems to make the fix work also on Windows. (thanks to WizardCM for the collab) ### Motivation and Context The keypress event was inusable on obs-browser. This seems to fully fix this Fixes #200 ### How Has This Been Tested? Tested on Slackware-current, using CEF build 3809. As long as I tested, all seems to work fine ### Types of changes <!--- What types of changes does your PR introduce? Uncomment all that apply --> Bug fix (non-breaking change which fixes an issue) <!--- - New feature (non-breaking change which adds functionality) --> <!--- - Performance enhancement (non-breaking change which improves efficiency) --> <!--- - Code cleanup (non-breaking change which makes code smaller or more readable) --> <!--- - Breaking change (fix or feature that would cause existing functionality to change) --> <!--- - Documentation (a change to documentation pages) --> ### Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [ ] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate.
kkartaltepe commented 2020-03-05 20:28:04 -06:00 (Migrated from github.com)

Please add the appropriate library search in cmake along with adding the appropriate include dirs (but without linking since we do not actually need the library).

Please add the appropriate library search in cmake along with adding the appropriate include dirs (but without linking since we do not actually need the library).
kkartaltepe commented 2020-03-06 10:27:47 -06:00 (Migrated from github.com)

You can see a good example of a simple find module at
https://github.com/obsproject/obs-studio/blob/master/cmake/Modules/FindX11_XCB.cmake

Adding a module searching pkg-config for x11(case matters) will get you the PKG_X11_LIBRARIES and PKG_X11_INCLUDE_DIR and follow the pattern in the above file for filling out the variables.

Like with findCef just drop it in the root of this package https://github.com/obsproject/obs-browser/blob/master/FindCEF.cmake

Then in the CMakeLists.txt you can simply find_package(X11) and use the X11_INCLUDE_DIR in an include_directories($X11_INCLUDE_DIR) in some Linux specific if block.

You can see a good example of a simple find module at https://github.com/obsproject/obs-studio/blob/master/cmake/Modules/FindX11_XCB.cmake Adding a module searching pkg-config for `x11`(case matters) will get you the PKG_X11_LIBRARIES and PKG_X11_INCLUDE_DIR and follow the pattern in the above file for filling out the variables. Like with findCef just drop it in the root of this package https://github.com/obsproject/obs-browser/blob/master/FindCEF.cmake Then in the CMakeLists.txt you can simply `find_package(X11)` and use the X11_INCLUDE_DIR in an `include_directories($X11_INCLUDE_DIR)` in some Linux specific `if` block.
Francexi commented 2020-03-11 07:40:52 -05:00 (Migrated from github.com)

I don't know if this is right, I think is how it should work

I don't know if this is right, I think is how it should work
kkartaltepe commented 2020-03-11 13:54:13 -05:00 (Migrated from github.com)

libX11-xcb is separate from libX11. The module for it is an example that is relatively simple. You should replace xcb references with plain x11 references. And change the header being searched for from the xcb header X11/Xlib-xcb.h to the X11 header you are using X11/keysym.h.

You can run cmake to test it. If running cmake with this commit didnt fail on your system you must already have an existing FindX11.cmake in your system module path that found it.

libX11-xcb is separate from libX11. The module for it is an example that is relatively simple. You should replace xcb references with plain x11 references. And change the header being searched for from the xcb header `X11/Xlib-xcb.h` to the X11 header you are using `X11/keysym.h`. You can run `cmake` to test it. If running cmake with this commit didnt fail on your system you must already have an existing FindX11.cmake in your system module path that found it.
Francexi commented 2020-03-12 04:41:21 -05:00 (Migrated from github.com)

this seems to go well. I also passed the files through clang-format

this seems to go well. I also passed the files through clang-format
kkartaltepe (Migrated from github.com) reviewed 2020-03-13 10:30:34 -05:00
CMakeLists.txt
kkartaltepe (Migrated from github.com) commented 2020-03-13 10:30:34 -05:00

This seems unrelated to the PR. Did you mean to add this change?

This seems unrelated to the PR. Did you mean to add this change?
Francexi (Migrated from github.com) reviewed 2020-03-13 10:42:46 -05:00
CMakeLists.txt
Francexi (Migrated from github.com) commented 2020-03-13 10:42:45 -05:00

woops, that was unintended. I corrected it

woops, that was unintended. I corrected it
kkartaltepe (Migrated from github.com) reviewed 2020-03-13 10:55:55 -05:00
obs-browser-source.cpp
@ -294,13 +298,19 @@ void BrowserSource::SendKeyClick(const struct obs_key_event *event, bool key_up)
{
kkartaltepe (Migrated from github.com) commented 2020-03-13 10:55:54 -05:00

If you are going to copy and paste code from chromium at least document it so we know where to go to find updated versions.

For example this entire function is out of date with chromium head and noted that it only sort of works correctly on US layouts.
https://chromium.googlesource.com/chromium/src/+/lkgr/ui/events/keycodes/keyboard_code_conversion_x.cc

If you are going to copy and paste code from chromium at least document it so we know where to go to find updated versions. For example this entire function is out of date with chromium head and noted that it only sort of works correctly on US layouts. https://chromium.googlesource.com/chromium/src/+/lkgr/ui/events/keycodes/keyboard_code_conversion_x.cc
Francexi (Migrated from github.com) reviewed 2020-03-13 11:01:17 -05:00
obs-browser-source.cpp
@ -294,13 +298,19 @@ void BrowserSource::SendKeyClick(const struct obs_key_event *event, bool key_up)
{
Francexi (Migrated from github.com) commented 2020-03-13 11:01:17 -05:00

well, that was the only mapping that I found that worked on both italian and US layouts (the two I had). If you want to suggest a modification in particular, can you do a pull request to my repo and I'll merge it

well, that was the only mapping that I found that worked on both italian and US layouts (the two I had). If you want to suggest a modification in particular, can you do a pull request to my repo and I'll merge it
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area/accessibility
Improves accessibility of the program

  
area/dependency
Issue is in external library or dependency, not OBS itself

  
Confirmed

   
Crash Report

   
Delayed
Merging has been delayed due to an issue

  
Design Issue
There is an issue with incorporating this pull request's design. It may take some time to finalize.

  
Duplicate

   
Feedback required
Issue does not contain enough information or is missing the template

  
Good first issue
New to contributing to OBS? Start here!

  
kind/bug
Categorizes issue or PR as related to a bug.

  
kind/cleanup
Non-breaking change which makes code smaller or more readable

  
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.

  
platform/linux

   
platform/macos

   
platform/windows

   
priority/high
This is an important PR

  
Question

   
Request for Comments
More feedback & discussion is requested

  
Seeking Other Contributors
This PR needs help from any other developers who are willing to give a crack at this code

  
Seeking Testers

   
Won't Fix / By Design
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/obs-browser!206
No description provided.