Created attachment 222390 [details] [review]
region: Drop the whitelist of supported IBus engines

This is the first patch of the series, dropping the whitelist. Next one filters out the XKB engines.

I hope I'm not stepping on anyone's toes with this, I just wanted this bug to make some progress, as it's necessary for bug 680840. :)

Created attachment 222391 [details] [review]
region: Exclude IBus XKB engines

Please review carefully though, I'm a Python developer and have almost zero experience in C. :)

Hey, thanks for the patches!

(In reply to comment #1)
> I hope I'm not stepping on anyone's toes with this, I just wanted this bug to
> make some progress, as it's necessary for bug 680840. :)

Sure, I just hadn't gotten around to it yet. You provided the extra motivation bit ;-)

(In reply to comment #3)
> Please review carefully though, I'm a Python developer and have almost zero
> experience in C. :)

I see, that explains why the memory management isn't right :-)

Also, I wanted to change this code to use the async ibus API to fetch the engines so I basically ended up re-doing the patch.

Review of attachment 222390 [details] [review]:

Removing from the patch queue.

Review of attachment 222391 [details] [review]:

Removing from the patch queue.

Created attachment 222429 [details] [review]
region: Replace the IBus engines whitelist with a blacklist instead

This also switches to the async engines getter.

--

I'm also blacklisting the m17n engines since those also add a lot of
noise to the list... We might have to let some of them through if
people come up with use cases for them.

Review of attachment 222429 [details] [review]:

::: panels/region/gnome-region-panel-input.c
@@ +187,3 @@
+
+      if (strncmp ("xkb:", engine_id, 4) == 0 ||
+          strncmp ("m17n:", engine_id, 5) == 0)

I would suggest to blacklist only "m17n:*:kbd" and a few duplicates: "m17n:zh:py", "m17n:ja:anthy", "m17n:ko:han2", and "m17n:ko:romaja".

They have been already hidden in Fedora quite a while and I can upstream the change if urgent.

I mean, s/upstream/fix in ibus-m17n side/.  Just filed a patch:

https://codereview.appspot.com/6485073/

Anyway, I don't think it is a good idea to hide all the m17n engines, because there are non-trivial m17n engines used (as default) in Southeast Asian countries like Thailand or Sri Lanka.  Currently those engines have no alternatives and cannot be replaced with XKB, since they use surrounding text natively:

https://bugzilla.redhat.com/show_bug.cgi?id=435880

and also some keycode might be assigned to multiple keysyms.

Thanks for the feedback. The main motivation here is to reduce duplication, while still letting all the actually used engines through. So, we should probably follow your proposal and tighten the m17n part of the blacklist.

Created attachment 222603 [details] [review]
blacklist m17n engines that are actually duplicated

*** Bug 682851 has been marked as a duplicate of this bug. ***

From bug 682851, you can see that it apparently causes problems with certain ibus engine names.

Created attachment 222694 [details] [review]
merge patch from bug 682851

It now switched back to use gdm_get_language_from_name() so that the code be consistent with other part of the region panel.  Sorry for the noise on 682851.

I'm not sure this is the best solution.  Other possible options are:

- use "English" (or locale language) instead of "Other" 
  as I wrote in 682851, "Other" needs to be translated

- simply skip the engines whose language is ""
  perhaps this might work since the most of such engines are kind of utilities which I expect not be used so often, though some people might use the latex input method daily to write mathematical papers

*** Bug 682851 has been marked as a duplicate of this bug. ***

The more I think about this the less I like changing this to a blacklist.

The way I see it, we are not doing an IBus UI, we are doing the Gnome input methods UI. The fact that there are engines for IBus like 'latex' and 'compose' doesn't mean that they make sense to expose from the Gnome point of view. They might make sense and be very useful but we really need a design on how to expose them. Just throwing everything into the "Region & Language" input sources chooser doesn't seem like the best decision IMHO.

So, I think that, at least for 3.6, we should keep using the whitelist. We should add more engines to it of course, at least the ones Mathieu mentioned in bug 680840 for traditional Chinese and make sure that the ones that we expose are actually usable and maybe even do specific tweaks to the way we present them on a case by case basis.

Daiki, do you known which engines are the most useful and work relatively well? Maybe "Thai (kesmanee (m17n))" is needed. Anything else?

Anyway, I'm sure users will report the lack of engines and we can keep adding them later if they really are important to a large enough population.

BTW, I separated the change to the async ibus engines getter into a different patch. That's bug 683035.

Sure, i guess thats a reasonable position and probably safer for 3.6.
Lets grow the whitelist, then.

FYI, Chromium OS has such a whitelist:

http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/browser/chromeos/input_method/input_methods.txt

But beware that, engines written in python (anthy and ibus-table) are not there, since they do not ship python in CrOS.  Instead, "mozc" (their product) and "m17n*" are used for CJK.

I agree having a default list of recommended IMEs is good,
but it really need to be made configurable.  If control-center
needs to be patched and rebuilt to add engines that is
really not nice for users and will make life extra hard
for distros - so I really hope the list can be handled
with gsettings?  Is that the plan or maybe I am missing something?

For Indic languages we have a page here -> https://fedoraproject.org/wiki/I18N/Indic#Keyboard_Layouts

there default m17n keymaps mean those are used more for that language but I don't think we should restrict to single map for Indic languages. We should add all the keymaps listed there.

(In reply to comment #19)
> I agree having a default list of recommended IMEs is good,
> but it really need to be made configurable.  If control-center
> needs to be patched and rebuilt to add engines that is
> really not nice for users and will make life extra hard
> for distros

Why would distros patch GNOME Control Center? Shouldn't they try to with upstream so sad needed engines become "well supported" and get added to the whitelist?

> - so I really hope the list can be handled
> with gsettings?  Is that the plan or maybe I am missing something?

Being able to add to the whitelist with GSettings might be overkill, perhaps a simple "disable_input_sources_whitelist" setting would be enough?

Or rather, couldn't GNOME Tweak Tool provide a list of "everything provided by IBus which is not in the whitelist" ?

This would keep the list in the Control Center short, simple, and only with "well supported" engines, while offering an easy path for users of "unsupported" engines. (until they become "well supported"?)

We already have a bug open to add a hidden gsettings for 'show extra xkb layouts' which would add all the fringe and exotic items to the add dialog. 
Should we reuse the same approach for ibus layouts ? Ie have a relatively small whitelist of engines that we know to work well, and then look at a gsetting to decide whether to let everything else in as well ?

I agree with Matthias. The bug he refers to is bug 682240 btw.

Created attachment 223232 [details] [review]
region: Honor the 'ibus-use-extra-engines' gsettings key

This setting overrides our supported IBus engines whitelist and makes
us show every engine installed on the system when enabled.

Created attachment 223382 [details] [review]
region: Honor the 'show-all-sources' gsettings key

--

Updated for the key change.

Review of attachment 223382 [details] [review]:

Looks good to me.

::: panels/region/gnome-region-panel-input.c
@@ +212,3 @@
     }
 
+  use_extra_engines = g_settings_get_boolean (input_sources_settings, "show-all-sources");

Pedantic nit: Might be nicer to rename to variable ot show_all_sources, to match the key name.

Review of attachment 223382 [details] [review]:

Same comment as Matthias.

Pushed with a renamed variable. Thanks

Attachment 223382 [details] pushed as 2a87480 - region: Honor the 'show-all-sources' gsettings key

I didn't mean to close this just yet.

Created attachment 223428 [details] [review]
region: Expand the supported IBus engines whitelist

Thanks to Daiki Ueno for assembling the list.

Review of attachment 223428 [details] [review]:

Looks good to me

Review of attachment 223428 [details] [review]:

Looks good to me

Thanks for the review.

Attachment 223428 [details] pushed as 9d46f83 - region: Expand the supported IBus engines whitelist

We got 61 m17n keymaps in this patch out of 169 keymaps provided by m17n* packages in Fedora. That's good but we are still missing at least single keymap for some other languages like Russia, Thai, Esperanto. Are we leaving it for now and planning to add in some later release or use "Show all" option for them?

(In reply to comment #34)
> We got 61 m17n keymaps in this patch out of 169 keymaps provided by m17n*
> packages in Fedora. That's good but we are still missing at least single keymap
> for some other languages like Russia, Thai, Esperanto. Are we leaving it for
> now and planning to add in some later release or use "Show all" option for
> them?

File a separate bug if particular keymaps are missing. For Esperanto and Russian at least, I'd expect them to be covered by XKB bindings.

*** Bug 680840 has been marked as a duplicate of this bug. ***