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

input: libinput: add setting to allow changing keymap layout #14341

Merged
merged 1 commit into from Oct 7, 2018

Conversation

@lrusak
Copy link
Contributor

commented Aug 23, 2018

This is a bit of a hack but it's needed in order for us to allow
using a different keyboard layout. The idea in the future is to
move libinput to a peripheral add-on but that won't be until
V19. So this is needed in the meantime.

I can also add more options in the future related to mice and keyboards
but I have only heard about keyboard layouts so far.

Even if we don't merge this because it's a bit invasive I'm ok with that and we can carry the patch for V18 in LibreELEC.

* See LICENSES/README.md for more information.
*/

#if defined(HAVE_GBM) || defined(TARGET_RASPBERRY_PI) || defined(HAS_LIBAMCODEC)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 23, 2018

Member

-10 for this ifdeffery

This comment has been minimized.

Copy link
@lrusak

lrusak Aug 23, 2018

Author Contributor

I can move this to CMake

if (setting == nullptr)
return;

#if defined(HAVE_GBM)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 23, 2018

Member

we are moving away form those kind of ifdef hacks

This comment has been minimized.

Copy link
@lrusak

lrusak Aug 23, 2018

Author Contributor

yea, not sure what to do here

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 23, 2018

Member

don't register the cb globally but locally in InputManager


CLibInputSettings::CLibInputSettings()
{
std::string xkbFile("/usr/share/X11/xkb/rules/base.xml");

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 23, 2018

Member

borrowing files from X11 when not using X11 is wrong

This comment has been minimized.

Copy link
@lrusak

lrusak Aug 23, 2018

Author Contributor

we use libxkbcommon which needs xkeyboard-config, there is no way around this


settingSet.clear();
settingSet.insert(CSettings::SETTING_INPUT_LIBINPUTKEYBOARDLAYOUT);
GetSettingsManager()->RegisterCallback(&CLibInputSettings::GetInstance(), settingSet);

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 23, 2018

Member

we don't register the handlers here anymore. check game, pvr, ae, etc. those register the callback locally.

@@ -362,6 +364,7 @@ const std::string CSettings::SETTING_AUDIOOUTPUT_DTSPASSTHROUGH = "audiooutput.d
const std::string CSettings::SETTING_AUDIOOUTPUT_TRUEHDPASSTHROUGH = "audiooutput.truehdpassthrough";
const std::string CSettings::SETTING_AUDIOOUTPUT_DTSHDPASSTHROUGH = "audiooutput.dtshdpassthrough";
const std::string CSettings::SETTING_AUDIOOUTPUT_VOLUMESTEPS = "audiooutput.volumesteps";
const std::string CSettings::SETTING_INPUT_LIBINPUTKEYBOARDLAYOUT = "input.libinputkeyboardlayout";

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 23, 2018

Member

no need to have this global, check AE, VAAPI, etc

@@ -15,7 +15,8 @@
#include <memory>
#include <vector>

class CLibInputKeyboard;
#include "LibInputKeyboard.h"

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 23, 2018

Member

inlude order is wrong, own headers before system headers

@@ -27,6 +28,8 @@ class CLibInputHandler : CThread

void Start();

bool SetKeymap(std::string layout) { return m_keyboard->SetKeymap(layout); }

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 23, 2018

Member

const std::string&

This comment has been minimized.

Copy link
@garbear

garbear Aug 23, 2018

Member

Move to .cpp file so you don't have to include LibInputKeyboard.h in this file. Always avoid pulling in #includes in the header when possible.

@@ -26,6 +26,8 @@ class CLibInputKeyboard
void UpdateLeds(libinput_device *dev);
void GetRepeat(libinput_device *dev);

bool SetKeymap(std::string layout);

This comment has been minimized.

Copy link
@Rechi

Rechi Aug 23, 2018

Member

const std::string&

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

cleaned up as requested, however I seem to be having an issue now where CSettings goes away and I get a segfault. Please advise. (do I even need to unregister?)

Thread 1 (Thread 0x7fc28b500e40 (LWP 23514)):
#0  0x0000000001e9ff0e in std::__uniq_ptr_impl<CSettings, std::default_delete<CSettings> >::_M_ptr() const (this=0x70) at /usr/include/c++/8/bits/unique_ptr.h:150
#1  0x0000000001e9e66a in std::unique_ptr<CSettings, std::default_delete<CSettings> >::get() const (this=0x70) at /usr/include/c++/8/bits/unique_ptr.h:343
#2  0x0000000001e9c8b4 in std::unique_ptr<CSettings, std::default_delete<CSettings> >::operator*() const (this=0x70) at /usr/include/c++/8/bits/unique_ptr.h:329
#3  0x0000000001e9ab42 in CServiceManager::GetSettings() (this=0x0) at /home/lukas/Documents/git/xbmc/xbmc/ServiceManager.cpp:335
#4  0x0000000001e98ca5 in CServiceBroker::GetSettings() () at /home/lukas/Documents/git/xbmc/xbmc/ServiceBroker.cpp:69
#5  0x0000000001672254 in CLibInputSettings::~CLibInputSettings() (this=0x5453c10, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/platform/linux/input/LibInputSettings.cpp:83
#6  0x0000000001672306 in CLibInputSettings::~CLibInputSettings() (this=0x5453c10, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/platform/linux/input/LibInputSettings.cpp:90
#7  0x000000000166913c in std::default_delete<CLibInputSettings>::operator()(CLibInputSettings*) const (this=0x5432518, __ptr=0x5453c10) at /usr/include/c++/8/bits/unique_ptr.h:81
#8  0x000000000166862b in std::unique_ptr<CLibInputSettings, std::default_delete<CLibInputSettings> >::~unique_ptr() (this=0x5432518, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/unique_ptr.h:274
#9  0x00000000016678f9 in CLibInputHandler::~CLibInputHandler() (this=0x5432220, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/platform/linux/input/LibInputHandler.cpp:97
#10 0x0000000001667944 in CLibInputHandler::~CLibInputHandler() (this=0x5432220, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/platform/linux/input/LibInputHandler.cpp:103
#11 0x0000000001639e2a in std::default_delete<CLibInputHandler>::operator()(CLibInputHandler*) const (this=0x53d32b8, __ptr=0x5432220) at /usr/include/c++/8/bits/unique_ptr.h:81
#12 0x0000000001638b17 in std::unique_ptr<CLibInputHandler, std::default_delete<CLibInputHandler> >::~unique_ptr() (this=0x53d32b8, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/unique_ptr.h:274
#13 0x000000000163f312 in CWinSystemGbm::~CWinSystemGbm() (this=0x53d31b0, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/windowing/gbm/WinSystemGbm.h:27
#14 0x000000000164d514 in CWinSystemGbmEGLContext::~CWinSystemGbmEGLContext() (this=0x53d31b0, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/windowing/gbm/WinSystemGbmEGLContext.h:20
#15 0x000000000164daae in CWinSystemGbmGLESContext::~CWinSystemGbmGLESContext() (this=0x53d31b0, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/windowing/gbm/WinSystemGbmGLESContext.h:22
#16 0x000000000164dad2 in CWinSystemGbmGLESContext::~CWinSystemGbmGLESContext() (this=0x53d31b0, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/windowing/gbm/WinSystemGbmGLESContext.h:22
#17 0x0000000001e0e0d8 in std::default_delete<CWinSystemBase>::operator()(CWinSystemBase*) const (this=0x5331ce8, __ptr=0x53d31b0) at /usr/include/c++/8/bits/unique_ptr.h:81
#18 0x0000000001e0ca43 in std::unique_ptr<CWinSystemBase, std::default_delete<CWinSystemBase> >::~unique_ptr() (this=0x5331ce8, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/unique_ptr.h:274
#19 0x0000000001df2061 in CApplication::~CApplication() (this=0x5331970, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/Application.cpp:244
#20 0x0000000001df217a in CApplication::~CApplication() (this=0x5331970, __in_chrg=<optimized out>) at /home/lukas/Documents/git/xbmc/xbmc/Application.cpp:249
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

Yes, you need to unregister

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

I'll have to investigate more after work today, but something isn't making sense to me.

It looks like we destroy windowing before the settings should be destroyed
https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L2704
https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L2752

what gives?

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

Looks like the winsystem deleter isn't called until way later.

This is a solution. It doesn't handle the problem of referencing the object but it's a solution that works lrusak@b10d63a

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

That would be three steps back to where we came from

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

We already have a solution for this. ServiceBroker should return a pointer that can be checked by the caller

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

We already have a solution for this. ServiceBroker should return a pointer that can be checked by the caller

Sure, but this isn't implemented here and even if it was I would just be adding dead code because it would always be null at this point in the shutdown anyways

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

Please pastenin a bt

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018


CLibInputSettings::~CLibInputSettings()
{
/* This currently will cause a segfault upon shutdown as the settings have gone away before windowing */

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 2, 2018

Member

If GetSettings() returns a pointer, you can test it for not being nullptr.

This comment has been minimized.

Copy link
@lrusak

lrusak Sep 3, 2018

Author Contributor

but it doesn't return a pointer

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 3, 2018

Member

The fix is to change that

This comment has been minimized.

Copy link
@lrusak

lrusak Sep 3, 2018

Author Contributor

same answer as before,

I would just be adding dead code because it would always be null at this point in the shutdown anyways

So yes, that is the right direction but doesn't necessarily solve this problem.

This comment has been minimized.

Copy link
@lrusak

lrusak Sep 4, 2018

Author Contributor

This seems to fix it also, lrusak@91dbdd7

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

The direction is that we return pointers in ServiceBroker. That would definitely fix the issue showm by your bt.
Quick hacks that rely on the order of init/deinit will break sooner or later

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

Some thoughts:

  1. You want me to unregister the callback properly.
  2. in order for the callback to unregister CSettings needs to exist when m_pWinSystem is deleted.
  3. Currently m_pWinSystem lives as long as CApplication does.
  4. Using a pointer in CServiceBroker will not solve this problem, it will simply mask it and the callback will never be unregistered properly.

So what is the proper solution?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

Please elaborate why returning a pointer does not solve the issue. The bt you posted shows that you access settings at a time it has already gone. A pointer would solve this

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

Why should I even unregister then if it's never going to happen?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

It will happen. Gui and windowing are supposed to come and go at any time.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

@FernetMenta so I went down the road to make GetSettings() return a pointer but it still won't work because m_serviceManager is reset before m_pWinSystem (because m_pWinSystem lasts as long as CApplication). So again, I'm not sure what the correct solution is.

So simply adding m_pWinSystem.reset() will fix the issue regardless of what happens

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

I only have a phone here and can‘t link you more info or code snippets. Please add the pointer commit to this pr. I can help with the rest when back home.
If serviceManager is still involved, there is something missing.

@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

Ok, I think I did it right, and it seems to work now, see lrusak@1be0b76

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

Almost there. ServiceBroker should return a shared ptr so it can be unregistered safely. Check appPort

@lrusak lrusak force-pushed the lrusak:libinput-settings branch from 63bbad4 to 78dd39e Sep 9, 2018

class CLibInputHandler;

static const std::string SETTING_INPUT_LIBINPUTKEYBOARDLAYOUT{"input.libinputkeyboardlayout"};

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 9, 2018

Member

why don't you add this to CLibInputSettings? I see no need for a global here

This comment has been minimized.

Copy link
@lrusak

lrusak Sep 9, 2018

Author Contributor

it's used in CLibInputKeyboard

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 10, 2018

Member

I mean it should be used like this:
CLibInputSettings::SETTING_INPUT_LIBINPUTKEYBOARDLAYOUT

@MartijnKaijser MartijnKaijser removed this from the Leia 18.0-beta2 milestone Sep 9, 2018
#include <algorithm>

const std::string CLibInputSettings::SETTING_INPUT_LIBINPUTKEYBOARDLAYOUT = "input.libinputkeyboardlayout";
static std::vector<std::pair<std::string, std::string>> m_layouts;

This comment has been minimized.

Copy link
@Rechi

Rechi Sep 13, 2018

Member

static isn't a member of a class, so m_ prefix is wrong

return;
}

const TiXmlElement* layoutElement = rootElement->FirstChildElement("layoutList")->FirstChildElement("layout");

This comment has been minimized.

Copy link
@garbear

garbear Sep 14, 2018

Member

Nested elements need null checks in TinyXML

This comment has been minimized.

Copy link
@lrusak

lrusak Sep 15, 2018

Author Contributor

thanks, fixed

}

std::string layout = configElement->FirstChildElement("name")->GetText();
std::string layoutDescription = configElement->FirstChildElement("description")->GetText();

This comment has been minimized.

Copy link
@garbear

garbear Sep 16, 2018

Member

I think every pointer returned by tinyxml needs a null check

This comment has been minimized.

Copy link
@lrusak

lrusak Sep 16, 2018

Author Contributor

done

@lrusak lrusak force-pushed the lrusak:libinput-settings branch 2 times, most recently from 4811913 to 0a66515 Sep 23, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

any other objections before merging?

#: system/settings/gbm.xml
#: xbmc/platforms/linux/input/LibInputSettings.cpp
msgctxt "#35030"
msgid "Keyboard"

This comment has been minimized.

Copy link
@garbear

garbear Oct 2, 2018

Member

This is already string 35150.

This is needed in order for us to allow using a different
keyboard layout. The idea in the future is to move libinput
to a peripheral add-on but that won't be until V19. So this
is needed in the meantime.

I can also add more options in the future related to mice and keyboards
but I have only heard about keyboard layouts so far.
@lrusak lrusak force-pushed the lrusak:libinput-settings branch from 0a66515 to e3eb33f Oct 2, 2018
@lrusak

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

Any other objections before merging?

@lrusak lrusak merged commit 8f8bbe3 into xbmc:master Oct 7, 2018
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@Rechi Rechi added this to the Leia 18.0-beta4 milestone Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.