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

feat(linux): Install keyboard on Gnome #3278

Merged
merged 1 commit into from Jun 25, 2020
Merged

Conversation

ermshiperete
Copy link
Contributor

This change adds the keyboard to the Gnome input sources after
installing the .kmx file, and removes it on uninstall.

This implements #2728.

This change adds the keyboard to the Gnome input sources after
installing the .kmx file, and removes it on uninstall.

This implements #2728.
@ermshiperete ermshiperete self-assigned this Jun 24, 2020
@ermshiperete ermshiperete added this to the P10S9 milestone Jun 24, 2020
@ermshiperete ermshiperete linked an issue Jun 24, 2020 that may be closed by this pull request
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

Small changes requested but looks great to me!

return []

values = []
# a(ss)
Copy link
Member

Choose a reason for hiding this comment

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

Can you elucidate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# a(ss)
nChildren = variant.n_children()
for i in range(nChildren):
# (ss)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment either :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

kmx_file = os.path.join(packageDir, keyboard['id'] + ".kmx")
if not ignore_language and "languages" in keyboard and len(keyboard["languages"]) > 0:
logging.debug(keyboard["languages"][0])
return "%s:%s" % (keyboard["languages"][0]['id'], kmx_file)
Copy link
Member

Choose a reason for hiding this comment

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

So this always returns the first language in the keyboard metadata? This seems to be an obvious area for future enhancement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. To the user this shows up as a confusing display of the keyboard in the Gnome UI. I'll create an issue. At the moment we can't improve this unless we know the language the user searched for (cf #1456)

Copy link
Member

Choose a reason for hiding this comment

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

We're going to improve the install workflow -- in the package install dialog, we should show a selector for each keyboard to choose from the list of languages in the package; if the default can be prepulated from the search per #1456, then that's ideal, but otherwise we still get a better answer.

__is_gnome_shell = None


def get_keyboard_id(keyboard, packageDir, ignore_language=False):
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a different name for this function? Rationale: keyboard_id is already a fairly heavily used term in Keyman (and equates to the basename of the keyboard file sans extension (e.g. khmer_angkor).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


# uninstall all kmx for all languages
for kb in keyboards:
keyboard_id = get_keyboard_id(kb, packageDir)
Copy link
Member

Choose a reason for hiding this comment

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

Please rename keyboard_id to something that qualifies it as a gnome_keyboard_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Named it ibus_keyboard_id.

@ermshiperete ermshiperete merged commit 7c9b13d into master Jun 25, 2020
@ermshiperete ermshiperete deleted the feat/issue-2728 branch June 25, 2020 14:00
ermshiperete added a commit that referenced this pull request Jun 26, 2020
feat(linux): address code review comments of #3278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[linux] km-config to add keyboards to ibus
2 participants