Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

RFC: [gui][confluence] support for international on-screen keyboards #4242

Closed
wants to merge 2 commits into from

4 participants

@vkosh

There are many cases and requests for i18l on-screen keyboard. This is an attempt to support it.
The keyboard layout and contents are defined in static data section of hidden container control in DialogKeyboard.xml, so it's up to a skinner to change it the way he wants (ABCDE, QWERTY, etc.). I did the definitions for default language (English) and for Russian.
User can switch layout between default language and local one. If local language equals to default then the switch button is disabled.
I don't know if this technique covers hieroglyphic languages, but at least it's a start of the keyboard internationalization.
Here are some screenshots:
en-en
ru-eng
ru-ru

@jmarshallnz
Owner

A good start, but we'll definitely need those key layouts and button labels out of the skin.

@vkosh

@jmarshallnz I thought about this, and my first implementation was with alphabet, digits and symbols in strings.po, and skin had a panel control, which was filled with the chars. But I found that some skins use qwerty layout (skin.touched) and the implementation doesn't fit.
So there must be a way to map chars to controls in skin.
The current implementation do this mapping and it's flexible in a way that skinner can use different controls for different languages/layouts. It's not implemented yet, but it's easy to handle for each layout separately.

@vkosh

Another problem is different char entities for the languages. E.g. I use <digits>+<alpha> for English, and only <alpha> for Russian to save place, as the latter contains 33 symbols (4 rows). I couldn't figure out how to put such a heterogeneous data in strings.po and handle them in different skins.

@jmarshallnz
Owner

The layout in terms of how the buttons are placed onscreen is up to the skin, but the mapping of labels to the buttons should be determined by core. Likewise, IMO the abc versus qwerty should be determined by core.

You don't want it in strings.po as it's not really translatable information: Rather it's a mapping of alphanumeric english characters to whatever the other language is. i.e. this is a keyboard layout thing, not a language thing (the layout and language are linked, in that some layouts only make sense for some languages, but you may have more than one keyboard layout that applies to the one language, or a keyboard layout that applies to more than one language).

A start would be to just pop the keyboard mapping as a static array in the c++. This would get it working in a skin-agnostic manner. You could include the abc + qwerty layouts as well for example. Obviously we'd want it to end up to be more extensible, via a setting for Keyboard layout under Localization which is auto-filled from some XML or similar.

@t-nelson
@t-nelson
@vkosh

@jmarshallnz @t-nelson thanks for the ideas. I ended up with two xmls: https://gist.github.com/vkosh/9102882
system/keymaps/layout.xml - that's what user can choose for specific language in settings,
skin/KeyboardLayoutMap.xml - skin specific mapping between controls and layouts.

@vkosh

I don't like actually the skin/KeyboardLayoutMap.xml mapping, although it proposes a less invasive way for keyboard internationalization.
So we have system/keymaps/layout.xml - abstraction for the layouts, which defines charsets. And to map the chars to the controls I propose to define some group control with predefined id in a skin, whose childs are filled with the chars by core.
This means that skin.confluence keyboard will have at least 6 rows (including row with space button) of buttons to place <digits>+<ru alphabet>.
@jmarshallnz @t-nelson what do you think?

@vkosh

... or instead of group control, controls may have predefined id's that uniquely mapped to a char in layout.
Say <control type="button" id="100"> - 0-th char in layout charset ("0" digit), id="110" - 10-th char ("a"/"A" - in ABC layout, or "q"/"Q" in QWERTY), etc.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone
@vkosh

A bit more thoughts about the layouts abstraction. I think there's no need to make layout selectable by user, and the actual keyboard layouting should be done by skin.
E.g. we have N rows * 10 buttons in confluence. If user selects ABC layout - that's ok in this skin, but QWERTY one is completely useless here, because we'll get:
row1: qwertyuiop (10 letters)
row2: asdfghjklz (10 letters)
row3: xcvbnm (6 letters)
instead of the correct one:
row1: qwertyuiop (10 letters)
row2: asdfghjkl (9 letters)
row3: zxcvbnm (7 letters)

So IMO there should be xml file with alphabets only (or in strings.po again) and controls-to-letters mapping somewere in skin-specific xml (e.g. skin/KeyboardLayoutMap.xml mentioned above).

@vkosh

The last try :)
Layouts definitions placed in KeyboardLayouts.xml file(s) (which may be placed under language directories).
Each layout represents visual model, where every key mapped to control (virtual key) with id.
Modifiers attribute of char may be extended in future to support complex layouts.
This is an extended version of skin/KeyboardLayoutMap.xml, but it's defined globally this time (not on skin level).
Skin should define all controls with ids defined in layout files.
@jmarshallnz @t-nelson are you ok with this?

@jmarshallnz
Owner

We're concentrating on Gotham atm, and this will definitely be for after Gotham, so we may be a little slower than usual to respond :)

It's a tricky one - as you point out you kinda want it skin-independent but at the same time it's dependent on how many buttons the skin has defined for the keyboard per-row. The best suggestion I have is that we define how many keys there should be per-row and skins need to comply with that (They tend to do this already, so I don't think it would be particularly burdensome).

As for the XML layout, I'd tend to try and make it as compact as you can so that it represents the layout a little more easily for human eyes. I'm not sure how this is best done, in particular whether you could just specify the rows all at once (10 chars or whatever).

@vkosh

The new implementation features:

  • Added support for multiple keyboard layouts per language. The layouts defined in langinfo.xml files (see definitions for en and ru). The layout format defines keys depending on selected modifier keys such as "shift" and "symbol".
  • Virtual keyboard can be switched between main and alternative layouts. They can be selected by user using locale.mainkeyboardlayout and local.altkeyboardlayout settings.
  • Confluence DialogKeyboard.xml reworked to support multiple layouts (see screenshots). 1 2 3
xbmc/dialogs/GUIDialogKeyboardGeneric.h
@@ -23,8 +23,10 @@
#include "guilib/GUIKeyboard.h"
#include "guilib/GUIDialog.h"
#include "utils/Variant.h"
+#include <set>
@jmarshallnz Owner

drop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/dialogs/GUIDialogKeyboardGeneric.cpp
((25 lines not shown))
}
- // set correct alphabet characters...
+ CKeyboardLayout *currentLayout =
@jmarshallnz Owner

const CKeyboardLayout &currentLayout maybe?

@vkosh
vkosh added a note

This leads to errors of passing "this" as const (even with const modifier for related KeyboardLayout getters).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/dialogs/GUIDialogKeyboardGeneric.cpp
((12 lines not shown))
{
- if (m_keyType == SYMBOLS)
- aLabel[0] = symbol_map[iButton - 48];
- else
- aLabel[0] = iButton;
- SetControlLabel(iButton, aLabel);
+ CGUIMessage msg(GUI_MSG_SELECTED, GetID(), CTL_BUTTON_LAYOUT);
+ OnMessage(msg);
+ }
+ else
+ {
+ CGUIMessage msg(GUI_MSG_DESELECTED, GetID(), CTL_BUTTON_LAYOUT);
@jmarshallnz Owner

You can probably simplify this to a 2-liner via a ternary on m_keyLayout. Not sure if there's a macro available for this common operation already - see GUIMessage.h

@vkosh
vkosh added a note

Yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/dialogs/GUIDialogKeyboardGeneric.cpp
@@ -103,6 +109,14 @@ void CGUIDialogKeyboardGeneric::OnInitWindow()
{
SET_CONTROL_HIDDEN(CTL_LABEL_HEADING);
}
+
+ m_keyLayout = MAIN;
+
+ SetProperty("LocaleLanguage", g_langInfo.GetLanguageLocale(true));
+ SetProperty("HasAltLayout", int((*g_langInfo.GetMainKeyboardLayout()) != (*g_langInfo.GetAltKeyboardLayout())));
@jmarshallnz Owner

Is this the only time you're using CKeyboardLayout::operator!= ? It seems to me that there'd be a more efficient method?

@vkosh
vkosh added a note

Removed operator!= and added HasAltKeyboardLayout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on the diff
xbmc/dialogs/GUIDialogKeyboardGeneric.cpp
((7 lines not shown))
#if defined(TARGET_DARWIN)
#include "osx/CocoaInterface.h"
#endif
-// Symbol mapping (based on MS virtual keyboard - may need improving)
-static char symbol_map[37] = ")!@#$%^&*([]{}-_=+;:\'\",.<>/?\\|`~ ";
+#define BUTTON_ID_OFFSET 100
+#define BUTTONS_PER_ROW 20
+#define BUTTONS_MAX_ROWS 4
@jmarshallnz Owner

It's a little unfortunate that the button layout has changed, which means a fair amount more work for skinners.

I guess there's probably little way around that if skins use quite different layouts (I dunno if they do?)

@vkosh
vkosh added a note

I tried to implement the layouts in less invasive way for skins in the start of the PR. But in current concept I can't see a way to implement layouts without breaking skins.

@jmarshallnz Owner

One way would be to check if button 65 is present (ASCII 'A') and if so set the appropriate symbol on each button in a backward compatible way. As you have the ascii ranges 65-91 and 48-57 to play with (36 keys) you could fill them pretty easily from the ABC English map by mapping 48-57 to 100-109, 65-73 to 120-128 etc.

My concern is that we need to give skinners a bit of time to adjust - there'll be a bunch of changes going into the skin now that Gotham is finalised, so allowing them to incrementally work on things would be useful, and I think it's achievable here without much code?

@vkosh
vkosh added a note

Thanks for the aiming. I'll think about this when we come to solution with modifiers and layouts switching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/LangInfo.h
@@ -35,6 +36,9 @@
class TiXmlNode;
+typedef std::map<std::string, CKeyboardLayout> MAP_KEYBOARD_LAYOUTS;
+typedef std::pair<std::string, CKeyboardLayout> PAIR_KEYBOARD_LAYOUTS;
+
@jmarshallnz Owner

Unless the types are needed outside the class, please leave them in the class.

Also, we tend to use LayoutMap style naming for these. The pairs one seems missing?

@vkosh
vkosh added a note

Removed pair and renamed map. Map type can be used outside by calling static LoadKeyboardLayouts.

@jmarshallnz Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on the diff
xbmc/LangInfo.cpp
@@ -693,3 +848,30 @@ void CLangInfo::SettingOptionsRegionsFiller(const CSetting *setting, std::vector
if (!match && regions.size() > 0)
current = regions[0];
}
+
+void CLangInfo::SettingOptionsKeyboardLayoutsFiller(const CSetting *setting, std::vector< std::pair<std::string, std::string> > &list, std::string &current)
+{
+ std::vector<std::string> names;
+ if (setting->GetId() == "locale.mainkeyboardlayout")
+ g_langInfo.GetMainKeyboardLayoutNames(names);
+ else if (setting->GetId() == "locale.altkeyboardlayout")
+ g_langInfo.GetAltKeyboardLayoutNames(names);
@jmarshallnz Owner

I'm not sure how many keyboard systems have support for separately configurable twin layouts. Most I've seen seem to just have a cycle. If you wanted to just use a cycle, you could use a multi-select list for it?

@vkosh
vkosh added a note

There are two fixed layouts because they are related to languages: main - for default language (English) and alternative - for user language. So user can't select more than two layouts.

@jmarshallnz Owner

Why limit it to two? Why have Main and Alt at all - instead just have a single reference for all currently loaded maps. The user can choose which ones they want displayed in the keyboard UI (if we feel having say 4-5 or whatever is too many to cycle through) from settings, but otherwise they can just cycle through them from a button on the keyboard, remembering the chosen one.

@vkosh
vkosh added a note

I agree, cycling is better in UI. As for more than two layouts (more than one per language) - I just can't imagine the use case. Why user whould want to switch between ABC and QWERTY or even more within one language layouts set?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/LangInfo.cpp
((34 lines not shown))
+{
+ MAP_KEYBOARD_LAYOUTS::iterator it = m_altKeyboardLayouts.find(strName);
+ if (it != m_altKeyboardLayouts.end())
+ m_altKeyboardLayout = &it->second;
+ else if (!m_altKeyboardLayouts.empty())
+ m_altKeyboardLayout = &m_altKeyboardLayouts.begin()->second;
+ else
+ m_altKeyboardLayout = m_mainKeyboardLayout;
+}
+
+void CLangInfo::GetAltKeyboardLayoutNames(std::vector<std::string> &array)
+{
+ for (MAP_KEYBOARD_LAYOUTS::const_iterator it = m_altKeyboardLayouts.begin(); it != m_altKeyboardLayouts.end(); it++)
+ {
+ array.push_back(it->first);
+ }
@jmarshallnz Owner

No need for the braces if it's never going to be extended (unlikely).

@vkosh
vkosh added a note

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on the diff
xbmc/LangInfo.cpp
((57 lines not shown))
+ iKeys |= MODIFIER_KEY_SYMBOL;
+ }
+ modifierKeysSet.insert(iKeys);
+ }
+ }
+
+ // parse keyboard rows
+ const TiXmlNode *pKeyboardRow = pKeyboard->FirstChild("row");
+ while (pKeyboardRow)
+ {
+ if (!pKeyboardRow->NoChildren())
+ {
+ std::string strRow = pKeyboardRow->FirstChild()->ValueStr();
+ std::wstring wstrRow;
+ g_charsetConverter.utf8ToW(strRow, wstrRow);
+ if (!modifierKeysSet.empty())
@jmarshallnz Owner

You only ever read it out as utf8, so why not store it as utf8?

@vkosh
vkosh added a note

I need std::wstring's to get utf8 character by its index. std::string works with bytes.

@jmarshallnz Owner

No you don't - you store std::wstring in the keyboard map, not wchar's. You then only ever use them to convert back to utf8.

@vkosh
vkosh added a note

Ok. I'm not a C++ programmer as you have probably noticed :) So if you suggest a solution I will appreciate.
I have utf8 string in std::string and I need to have utf8 std::string on output containing i'th character of s. Something like code below but for utf8:
std::string strIn = "...";
std::string strOut;
strOut = strIn.substr(i, 1);

@jmarshallnz Owner

I have a few PRs of my own to tidy up, but will try and take a look at changing a bit of stuff in here for you to look at if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/LangInfo.cpp
((71 lines not shown))
+ g_charsetConverter.utf8ToW(strRow, wstrRow);
+ if (!modifierKeysSet.empty())
+ {
+ for (std::set<unsigned int>::const_iterator it = modifierKeysSet.begin(); it != modifierKeysSet.end(); it++)
+ layout.AddKeyboardRow(wstrRow, *it);
+ }
+ else
+ layout.AddKeyboardRow(wstrRow);
+ }
+ pKeyboardRow = pKeyboardRow->NextSibling();
+ }
+
+ pKeyboard = pKeyboard->NextSiblingElement();
+ }
+
+ keyboardLayouts.insert(PAIR_KEYBOARD_LAYOUTS(layout.GetName(), layout));
@jmarshallnz Owner

make_pair() is your friend :)

@vkosh
vkosh added a note

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on the diff
xbmc/LangInfo.cpp
@@ -410,6 +428,95 @@ void CLangInfo::LoadTokens(const TiXmlNode* pTokens, vector<CStdString>& vecToke
}
}
+void CLangInfo::LoadKeyboardLayouts(const std::string& strFileName, MAP_KEYBOARD_LAYOUTS &keyboardLayouts)
+{
+ CXBMCTinyXML xmlDoc;
+ if (!xmlDoc.LoadFile(strFileName))
+ {
+ CLog::Log(LOGERROR, "unable to load %s: %s at line %d", strFileName.c_str(), xmlDoc.ErrorDesc(), xmlDoc.ErrorRow());
+ return;
+ }
@jmarshallnz Owner

This error doesn't clear the keyboardLayouts. Is this what you want?

@vkosh
vkosh added a note

Yes. The keyboard dialog don't work without layouts, so at least main one should present. The previous layouts should remain in case of error.

@jmarshallnz Owner

This problem would go away if CLangInfo held only the keyboard layout for the single language it represents. i.e. g_langInfo probably isn't the appropriate place to hold stuff from English if the language is set to Russian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/LangInfo.cpp
((8 lines not shown))
}
bool CLangInfo::Load(const std::string& strFileName, bool onlyCheckLanguage /*= false*/)
{
SetDefaults();
+ std::string strMainLangInfoPath = StringUtils::Format("special://xbmc/language/%s/langinfo.xml", m_defaultRegion.m_strLangLocaleName.c_str());
@jmarshallnz Owner

I don't think you want to do this except where onlyCheckLanguage is false?

You probably want this moved down to the block below?

@vkosh
vkosh added a note

Added onlyCheckLanguage check (not used though).
As for moving main layout loading down the locale specific langinfo.xml check - the main layout must be loaded in any case otherwise keyboard dialog will not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz commented on the diff
language/Russian/langinfo.xml
((29 lines not shown))
+ <row>йцукенгшщзхъ</row>
+ <row>фывапролджэ</row>
+ <row>ячсмитьбю</row>
+ </keyboard>
+ <keyboard modifiers="shift">
+ <row>Ё1234567890</row>
+ <row>ЙЦУКЕНГШЩЗХЪ</row>
+ <row>ФЫВАПРОЛДЖЭ</row>
+ <row>ЯЧСМИТЬБЮ</row>
+ </keyboard>
+ <keyboard modifiers="symbol,shift+symbol">
+ <row>)!@#$%^&*(</row>
+ <row>[]{}-_=+;:</row>
+ <row>'",.&lt;&gt;/?\|</row>
+ <row>`~</row>
+ </keyboard>
@jmarshallnz Owner

Another thing to consider perhaps is merging the shift + symbols button into a cycle?

@vkosh
vkosh added a note

IMO the separate buttons is better in two reasons: usability of shift button and ability of symbols charset extention (e.g. separate charsets for symbols and for shift+symbols).

@jmarshallnz Owner

The advantage of a cycle allows arbitrary numbers of maps, not just 3 or 4. Obviously the first cycle would be exactly the same as shift, so it should be just as quick I think for most things. Even for symbols it would just be 2 quick clicks on the cycle button to get to symbols (which would reset back again once you hit a main button).

@vkosh
vkosh added a note

Good point, but I still think that shift and symbols are quite a different kind of modifiers to merge them on one button. Shift just changes letters case while symbols turns on sublayout with totally different charset (so it's not a modifier actually).
I've found two kinds of virtual keyboards: with (for example http://goo.gl/B946t6) and without (http://goo.gl/LbwHGC) symbols switch. The first kind used when there is no much space to place digits and symbols with letters (that's our case), and the second one uses shift as the symbols switch for digits row.
We could add more symbols sets in the future (e.g. smiles or whatever) and use cycling for "symbols" button. But "true" modifiers should be represented as standalone buttons I think.

@jmarshallnz Owner

If we do have more symbol sets in future, that would kinda indicate that shift applies to letters but doesn't really apply to symbols (which is what we have now, as well as with your current implementation for Russian).

I'd be OK with that, but it does mean that you can simplify the setup a bit as the layout is just standard, shift, symbols, and we just treat shift as a special layout activated by the shift button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz
Owner

Nice work overall. I think if this can be cleaned up a bit it'll make a great addition for non-English speakers.

@jmarshallnz jmarshallnz commented on the diff
xbmc/LangInfo.cpp
((8 lines not shown))
}
bool CLangInfo::Load(const std::string& strFileName, bool onlyCheckLanguage /*= false*/)
{
SetDefaults();
+ if (!onlyCheckLanguage)
+ {
+ std::string strMainLangInfoPath = StringUtils::Format("special://xbmc/language/%s/langinfo.xml", m_defaultRegion.m_strLangLocaleName.c_str());
+ LoadKeyboardLayouts(strMainLangInfoPath, m_mainKeyboardLayouts);
@jmarshallnz Owner

You're referencing m_defaultRegion.m_strLangLocaleName here but it's promptly overridden about 20 lines down from here. By storing English and non-English in the same class you're asking for problems down the line.

I suspect what you want is the reading of the English setup to be done separately to this class. i.e. g_langInfo would hold any keyboard layouts for the specific language, while (if the language is something other than English) you load in the English layouts directly in the keyboard class.

@vkosh
vkosh added a note

I agree, but all loaded layouts should be accesible from the one place (e.g. for settings). On the other hand the second instance of CLangInfo is redundant for loading layouts only except of using separate method for this. The implementation will depend on chosen solution between two fixed/multiple user selected layouts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz
Owner

Closing in favour of #5010

@vkosh vkosh closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.