Add icon_symbol property in IBusEngineDesc.

Add icon_symbol property in IBusEngineDesc.


TEST=Linux desktop

[Peng, 2011-06-22 13:50:28]

On 2011/06/22 11:11:25, fujiwara wrote:

What's is icon symbol for? Could you give some detail? Thx

[fujiwara, 2011-06-23 02:21:37]

On 2011/06/22 13:50:28, Peng wrote:
> What's is icon symbol for? Could you give some detail? Thx

The icon symbol is for the text icon on IBus panel status icon.
GNOME-Shell supports both the image icon and text icon.
http://desktopi18n.wordpress.com/2011/06/23/ibus-icon-symbol-property/

[Peng, 2011-06-23 03:29:58]

On 2011/06/23 02:21:37, fujiwara wrote:
> On 2011/06/22 13:50:28, Peng wrote:
> > What's is icon symbol for? Could you give some detail? Thx
> 
> The icon symbol is for the text icon on IBus panel status icon.
> GNOME-Shell supports both the image icon and text icon.
> http://desktopi18n.wordpress.com/2011/06/23/ibus-icon-symbol-property/

I think use such kind of icon may confuse users. For example Chinese pinyin has
'简' and '繁' two modes. But other Chinese engines (wubi, xingma) has those two
modes. So if we use '简' and '繁' as icon, users will not know which engine is
used currently.

[fujiwara, 2011-06-23 03:54:10]

On 2011/06/23 03:29:58, Peng wrote:
> I think use such kind of icon may confuse users. For example Chinese pinyin
has
> '简' and '繁' two modes. But other Chinese engines (wubi, xingma) has those two
> modes. So if we use '简' and '繁' as icon, users will not know which engine is
> used currently.

Hmm.., I think it's the different topic? The main purpose is to use the text
icon instead of images. AFAIK, ibus-pinyin's image shows a character using a
image?
The current suggestion for GNOME3 is to use the text icon:
https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage
AFAIK, one of their concerns is that each engine's icon uses many colors and
probably the text icon would be nice for the GNOME 3 theme.

[Peng, 2011-06-23 04:02:40]

https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c
File bus/engineproxy.c (right):

https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c#newcode594
bus/engineproxy.c:594: if (g_strcmp0 (signal_name, "SetIconSymbol") == 0) {
Is it necessary to allow engine change the symbol on the fly?

https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c
File src/ibusenginedesc.c (right):

https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode43
src/ibusenginedesc.c:43: PROP_ICON_SYMBOL,
Maybe it is better to rename icon-symbol to symbol

https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode411
src/ibusenginedesc.c:411: /* append extra properties */
maybe we could just add a string to builder. Does it have any problem?

[fujiwara, 2011-06-23 04:35:39]

On 2011/06/23 04:02:40, Peng wrote:
> https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c
> File bus/engineproxy.c (right):
> 
> https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c#newcode594
> bus/engineproxy.c:594: if (g_strcmp0 (signal_name, "SetIconSymbol") == 0) {
> Is it necessary to allow engine change the symbol on the fly?

$S-Windows IME also shows the input mode besides the IME symbol and we think it
would be nice.

> 
> https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c
> File src/ibusenginedesc.c (right):
> 
> https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode43
> src/ibusenginedesc.c:43: PROP_ICON_SYMBOL,
> Maybe it is better to rename icon-symbol to symbol

Currenty 'icon' property exists and both 'icon' and 'icon-symbol' is used for
the panel icon.
I thought 'icon-symbol' could give the image of the short description than
'symbol'.
Do you prefer 'symbol' to 'icon-symbol' and 'symbol' could give the short
description?

> https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode411
> src/ibusenginedesc.c:411: /* append extra properties */
> maybe we could just add a string to builder. Does it have any problem?

Previously we encounter SEGV in ibus-qt and I think using dict could avoid the
problem since the receiver need to know the property name and the back
compatibility would be kept without rebuilds.

[Peng, 2011-06-23 19:57:29]

On 2011/06/23 04:35:39, fujiwara wrote:
> On 2011/06/23 04:02:40, Peng wrote:
> > https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c
> > File bus/engineproxy.c (right):
> > 
> > https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c#newcode594
> > bus/engineproxy.c:594: if (g_strcmp0 (signal_name, "SetIconSymbol") == 0) {
> > Is it necessary to allow engine change the symbol on the fly?
> 
> $S-Windows IME also shows the input mode besides the IME symbol and we think
it
> would be nice.

Are they same? You plan show icon and the symbol together. I thought symbol here
is only for replacing the icon in gnome-shell. 

> 
> > 
> > https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c
> > File src/ibusenginedesc.c (right):
> > 
> > https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode43
> > src/ibusenginedesc.c:43: PROP_ICON_SYMBOL,
> > Maybe it is better to rename icon-symbol to symbol
> 
> Currenty 'icon' property exists and both 'icon' and 'icon-symbol' is used for
> the panel icon.
> I thought 'icon-symbol' could give the image of the short description than
> 'symbol'.
> Do you prefer 'symbol' to 'icon-symbol' and 'symbol' could give the short
> description?

I personally think symbol is better. I am also thinking to use icon property.
For example. The pinyin engine, we could use
‘ibus-pinyin:/full_path/ibus-pinyin.svg:拼’
What do you think?

> 
> >
https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode411
> > src/ibusenginedesc.c:411: /* append extra properties */
> > maybe we could just add a string to builder. Does it have any problem?
> 
> Previously we encounter SEGV in ibus-qt and I think using dict could avoid the
> problem since the receiver need to know the property name and the back
> compatibility would be kept without rebuilds.

I think using a dict may have the same problem.

[fujiwara, 2011-06-24 05:28:08]

On 2011/06/23 19:57:29, Peng wrote:
> On 2011/06/23 04:35:39, fujiwara wrote:
> > On 2011/06/23 04:02:40, Peng wrote:
> > > https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c
> > > File bus/engineproxy.c (right):
> > > 
> > > https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c#newcode594
> > > bus/engineproxy.c:594: if (g_strcmp0 (signal_name, "SetIconSymbol") == 0)
{
> > > Is it necessary to allow engine change the symbol on the fly?
> > 
> > $S-Windows IME also shows the input mode besides the IME symbol and we think
> it
> > would be nice.
> 
> Are they same? You plan show icon and the symbol together. I thought symbol
here
> is only for replacing the icon in gnome-shell. 

Yes, I think they are same.
I thought your concern again.
I checked gnome-settings-daemon icon on gnome-panel.
It shows languages for layouts and if there are duplicated, numbers are added.
E.g. 'en', 'en2', 'en3'.
I think that way could resolve your concerns too.
As I talked off line, currently we will try to change the definition of Control
+ Space.
Then the original problem was hard to different IM on and off because both
language is same. So I thought keyboard layout uses language symbols and input
method uses multi-byte symbols.

If two IM engines could show the same language, we also could add numbers for
input modes:
'あ', 'あ2', 'あ3'... and 'ア', 'ア2', 'ア3'...
Now I also think the number implementation would be in ibus panel and
gnome-shell panel instead of ibus-daemon?
So I'm thinking the patch does not need to be modified for your concern.
Currently PyGTK doesn't support text icon so I will implement the numbers in
gnome-shell only.

So gnome-shell icon shows layout languages and IM symbols and input modes and if
they have a same language, numbers are added.
The input modes are shown only when a engine calls
ibus.Engine.set_icon_symbol().

Does it make sense?

From the design of
http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage,
it seems desktop team doesn't mind the duplicated languages on the panel so
probably I need to talk with gnome-shell later with an updated patch.

> > > https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c
> > > File src/ibusenginedesc.c (right):
> > > 
> > >
https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode43
> > > src/ibusenginedesc.c:43: PROP_ICON_SYMBOL,
> > > Maybe it is better to rename icon-symbol to symbol
> > 
> > Currenty 'icon' property exists and both 'icon' and 'icon-symbol' is used
for
> > the panel icon.
> > I thought 'icon-symbol' could give the image of the short description than
> > 'symbol'.
> > Do you prefer 'symbol' to 'icon-symbol' and 'symbol' could give the short
> > description?
> 
> I personally think symbol is better. I am also thinking to use icon property.
> For example. The pinyin engine, we could use
> ‘ibus-pinyin:/full_path/ibus-pinyin.svg:拼’
> What do you think?

I have a concern using 'icon' property regarding to the back compatibility.
Currently I'm thinking the following patch to check the support:
http://fujiwara.fedorapeople.org/ibus/20110624/ibus-anthy-xx-icon-symbol.patch

If we use the 'icon' property, engines don't know if ibus supports the symbol
extension.

> https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode411
> > > src/ibusenginedesc.c:411: /* append extra properties */
> > > maybe we could just add a string to builder. Does it have any problem?
> > 
> > Previously we encounter SEGV in ibus-qt and I think using dict could avoid
the
> > problem since the receiver need to know the property name and the back
> > compatibility would be kept without rebuilds.
> 
> I think using a dict may have the same problem.

I don't understand the problem of the dict. I'll talk with you later.

[Peng, 2011-06-28 04:18:24]

On 2011/06/24 05:28:08, fujiwara wrote:
> On 2011/06/23 19:57:29, Peng wrote:
> > On 2011/06/23 04:35:39, fujiwara wrote:
> > > On 2011/06/23 04:02:40, Peng wrote:
> > > > https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c
> > > > File bus/engineproxy.c (right):
> > > > 
> > > >
https://codereview.appspot.com/4648050/diff/1/bus/engineproxy.c#newcode594
> > > > bus/engineproxy.c:594: if (g_strcmp0 (signal_name, "SetIconSymbol") ==
0)
> {
> > > > Is it necessary to allow engine change the symbol on the fly?
> > > 
> > > $S-Windows IME also shows the input mode besides the IME symbol and we
think
> > it
> > > would be nice.
> > 
> > Are they same? You plan show icon and the symbol together. I thought symbol
> here
> > is only for replacing the icon in gnome-shell. 
> 
> Yes, I think they are same.
> I thought your concern again.
> I checked gnome-settings-daemon icon on gnome-panel.
> It shows languages for layouts and if there are duplicated, numbers are added.
> E.g. 'en', 'en2', 'en3'.
> I think that way could resolve your concerns too.
> As I talked off line, currently we will try to change the definition of
Control
> + Space.
> Then the original problem was hard to different IM on and off because both
> language is same. So I thought keyboard layout uses language symbols and input
> method uses multi-byte symbols.
> 
> If two IM engines could show the same language, we also could add numbers for
> input modes:
> 'あ', 'あ2', 'あ3'... and 'ア', 'ア2', 'ア3'...
> Now I also think the number implementation would be in ibus panel and
> gnome-shell panel instead of ibus-daemon?
> So I'm thinking the patch does not need to be modified for your concern.
> Currently PyGTK doesn't support text icon so I will implement the numbers in
> gnome-shell only.
> 
> So gnome-shell icon shows layout languages and IM symbols and input modes and
if
> they have a same language, numbers are added.
> The input modes are shown only when a engine calls
> ibus.Engine.set_icon_symbol().
> 
> Does it make sense?

I am not sure if it is a good idea to allow engine change the symbol and use it
for internal input modes. So I think it is better to remove SetIconSymbol in
this CL. If we need show internal input mode in panel, I think we should add
another icon for it.


> 
> From the design of
> http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage,
> it seems desktop team doesn't mind the duplicated languages on the panel so
> probably I need to talk with gnome-shell later with an updated patch.
> 
> > > > https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c
> > > > File src/ibusenginedesc.c (right):
> > > > 
> > > >
> https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode43
> > > > src/ibusenginedesc.c:43: PROP_ICON_SYMBOL,
> > > > Maybe it is better to rename icon-symbol to symbol
> > > 
> > > Currenty 'icon' property exists and both 'icon' and 'icon-symbol' is used
> for
> > > the panel icon.
> > > I thought 'icon-symbol' could give the image of the short description than
> > > 'symbol'.
> > > Do you prefer 'symbol' to 'icon-symbol' and 'symbol' could give the short
> > > description?
> > 
> > I personally think symbol is better. I am also thinking to use icon
property.
> > For example. The pinyin engine, we could use
> > ‘ibus-pinyin:/full_path/ibus-pinyin.svg:拼’
> > What do you think?
> 
> I have a concern using 'icon' property regarding to the back compatibility.
> Currently I'm thinking the following patch to check the support:
> http://fujiwara.fedorapeople.org/ibus/20110624/ibus-anthy-xx-icon-symbol.patch
> 
> If we use the 'icon' property, engines don't know if ibus supports the symbol
> extension.
> 
> >
https://codereview.appspot.com/4648050/diff/1/src/ibusenginedesc.c#newcode411
> > > > src/ibusenginedesc.c:411: /* append extra properties */
> > > > maybe we could just add a string to builder. Does it have any problem?
> > > 
> > > Previously we encounter SEGV in ibus-qt and I think using dict could avoid
> the
> > > problem since the receiver need to know the property name and the back
> > > compatibility would be kept without rebuilds.
> > 
> > I think using a dict may have the same problem.
> 
> I don't understand the problem of the dict. I'll talk with you later.

What's the the problem for just add a integer? The problem of dict should be
same. :)

[fujiwara, 2011-06-29 08:28:40]

OK, I updated the patch.

On 2011/06/28 04:18:24, Peng wrote:
> I am not sure if it is a good idea to allow engine change the symbol and use
it
> for internal input modes. So I think it is better to remove SetIconSymbol in
> this CL. If we need show internal input mode in panel, I think we should add
> another icon for it.

After I talked off line, it seems the concern might be the definition of the
engine description.

> I am not sure if it is a good idea to allow engine change the 
> symbol dynamically. Because it is designed for indicating the 
> engine, not for the internal input mode

We also talked if ibus.Property would have the key of "InputMode" about the
input mode.
I removed the set_icon_symbol function here and will think about ibus.Property
later.

> What's the the problem for just add a integer? The problem of dict should be
> same. :)

IBus serialize/deserialize depends on the order so if the order is changed, ibus
crashes. using dict could avoid this issue since other components could not get
the property if the name is not known and dict doesn't have the order.
Using dict could avoid the issue because it checks g_strcmp0(key, "symbol") and
if it's not matched, the value is simply ignored.

Finally we agreed we will not change the serialized order for the compatibility.
I removed the dict here.

[Peng, 2011-07-03 19:28:10]

On 2011/06/29 08:28:40, fujiwara wrote:
> OK, I updated the patch.
> 
> On 2011/06/28 04:18:24, Peng wrote:
> > I am not sure if it is a good idea to allow engine change the symbol and use
> it
> > for internal input modes. So I think it is better to remove SetIconSymbol in
> > this CL. If we need show internal input mode in panel, I think we should add
> > another icon for it.
> 
> After I talked off line, it seems the concern might be the definition of the
> engine description.
> 
> > I am not sure if it is a good idea to allow engine change the 
> > symbol dynamically. Because it is designed for indicating the 
> > engine, not for the internal input mode
> 
> We also talked if ibus.Property would have the key of "InputMode" about the
> input mode.
> I removed the set_icon_symbol function here and will think about ibus.Property
> later.
> 
> > What's the the problem for just add a integer? The problem of dict should be
> > same. :)
> 
> IBus serialize/deserialize depends on the order so if the order is changed,
ibus
> crashes. using dict could avoid this issue since other components could not
get
> the property if the name is not known and dict doesn't have the order.
> Using dict could avoid the issue because it checks g_strcmp0(key, "symbol")
and
> if it's not matched, the value is simply ignored.
> 
> Finally we agreed we will not change the serialized order for the
compatibility.
> I removed the dict here.

LGTM

[Peng, 2011-07-04 17:38:15]

On 2011/07/03 19:28:10, Peng wrote:
> On 2011/06/29 08:28:40, fujiwara wrote:
> > OK, I updated the patch.
> > 
> > On 2011/06/28 04:18:24, Peng wrote:
> > > I am not sure if it is a good idea to allow engine change the symbol and
use
> > it
> > > for internal input modes. So I think it is better to remove SetIconSymbol
in
> > > this CL. If we need show internal input mode in panel, I think we should
add
> > > another icon for it.
> > 
> > After I talked off line, it seems the concern might be the definition of the
> > engine description.
> > 
> > > I am not sure if it is a good idea to allow engine change the 
> > > symbol dynamically. Because it is designed for indicating the 
> > > engine, not for the internal input mode
> > 
> > We also talked if ibus.Property would have the key of "InputMode" about the
> > input mode.
> > I removed the set_icon_symbol function here and will think about
ibus.Property
> > later.
> > 
> > > What's the the problem for just add a integer? The problem of dict should
be
> > > same. :)
> > 
> > IBus serialize/deserialize depends on the order so if the order is changed,
> ibus
> > crashes. using dict could avoid this issue since other components could not
> get
> > the property if the name is not known and dict doesn't have the order.
> > Using dict could avoid the issue because it checks g_strcmp0(key, "symbol")
> and
> > if it's not matched, the value is simply ignored.
> > 
> > Finally we agreed we will not change the serialized order for the
> compatibility.
> > I removed the dict here.
> 
> LGTM

I applied this CL. Please code it. Thanks.