Fix hiding button items in GTK panel.

Fix hiding button items in GTK panel.


BUG=none
TEST=manual

[Daiki Ueno, 2011-09-28 07:45:32]

self.hide_all() has no effect because "no_show_all" is set on the previous line.

[Peng, 2011-09-28 19:05:52]

https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py
File ui/gtk/toolitem.py (right):

https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py#newcode79
ui/gtk/toolitem.py:79: self.hide_all()
ditto

https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py#newcode134
ui/gtk/toolitem.py:134: self.hide_all()
it is better to use self.hide(). hide_all() was removed in gtk.

[Daiki Ueno, 2011-09-29 00:46:12]

It seems the code could be simplified more.  Since ToolButton and
ToggleToolButton are directly derived from gtk.ToolButton and
gtk.ToggleToolButton, show() and hide() (not *_all()) should be sufficient here.
 Also, there is no need to call set_no_show_all() each time showing/hiding the
widgets, as it is set in LanguageBar#register_properties().

After casual testing, the simplified version (CL 2) seems to work fine.

https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py
File ui/gtk/toolitem.py (right):

https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py#newcode79
ui/gtk/toolitem.py:79: self.hide_all()
On 2011/09/28 19:05:52, Peng wrote:
> ditto

Done.

https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py#newcode134
ui/gtk/toolitem.py:134: self.hide_all()
On 2011/09/28 19:05:52, Peng wrote:
> it is better to use self.hide(). hide_all() was removed in gtk.

Done.

[Peng, 2011-09-29 01:09:30]

On 2011/09/29 00:46:12, Daiki Ueno wrote:
> It seems the code could be simplified more.  Since ToolButton and
> ToggleToolButton are directly derived from gtk.ToolButton and
> gtk.ToggleToolButton, show() and hide() (not *_all()) should be sufficient
here.
>  Also, there is no need to call set_no_show_all() each time showing/hiding the
> widgets, as it is set in LanguageBar#register_properties().

I remember calling set_no_show_all() is for preventing the item becomes visible
again, when user hide and then reshow the panel.
 

> 
> After casual testing, the simplified version (CL 2) seems to work fine.
> 
> https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py
> File ui/gtk/toolitem.py (right):
> 
> https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py#newcode79
> ui/gtk/toolitem.py:79: self.hide_all()
> On 2011/09/28 19:05:52, Peng wrote:
> > ditto
> 
> Done.
> 
> https://codereview.appspot.com/5148041/diff/1/ui/gtk/toolitem.py#newcode134
> ui/gtk/toolitem.py:134: self.hide_all()
> On 2011/09/28 19:05:52, Peng wrote:
> > it is better to use self.hide(). hide_all() was removed in gtk.
> 
> Done.

[Daiki Ueno, 2011-09-29 01:34:07]

On 2011/09/29 01:09:30, Peng wrote:
> On 2011/09/29 00:46:12, Daiki Ueno wrote:
> > It seems the code could be simplified more.  Since ToolButton and
> > ToggleToolButton are directly derived from gtk.ToolButton and
> > gtk.ToggleToolButton, show() and hide() (not *_all()) should be sufficient
> here.
> >  Also, there is no need to call set_no_show_all() each time showing/hiding
the
> > widgets, as it is set in LanguageBar#register_properties().
> 
> I remember calling set_no_show_all() is for preventing the item becomes
visible
> again, when user hide and then reshow the panel.

Well, I suspect that it was necessary because the original code resets
"no_show_all" when showing:

if self._prop.visible:
    self.set_no_show_all(False)
    self.show_all()

because the "no_show_all" property affects self.show_all().  However, the new
code does not reset the property when showing, and self.show() works even if
"no_show_all" is True.

My test case is, with ibus-m17n and the patch to hide status button if status ==
title:

https://github.com/ueno/ibus-m17n/commit/390a2c7f

If I enable m17n anthy, type "a" and space, I see "漢" as the status prop.  Then,
when I type enter, I see the status prop is hidden, and Ctrl+Space twice to
hide/show the panel, the status prop is still hidden.

[Daiki Ueno, 2011-09-29 01:47:12]

On 2011/09/29 01:34:07, Daiki Ueno wrote:
> My test case is, with ibus-m17n and the patch to hide status button if status
==
> title:
> 
> https://github.com/ueno/ibus-m17n/commit/390a2c7f

Sorry, this patch is better and still works:

https://github.com/ueno/ibus-m17n/commit/d4a1cb9c

[Peng, 2011-09-29 01:54:42]

On 2011/09/29 01:34:07, Daiki Ueno wrote:
> On 2011/09/29 01:09:30, Peng wrote:
> > On 2011/09/29 00:46:12, Daiki Ueno wrote:
> > > It seems the code could be simplified more.  Since ToolButton and
> > > ToggleToolButton are directly derived from gtk.ToolButton and
> > > gtk.ToggleToolButton, show() and hide() (not *_all()) should be sufficient
> > here.
> > >  Also, there is no need to call set_no_show_all() each time showing/hiding
> the
> > > widgets, as it is set in LanguageBar#register_properties().
> > 
> > I remember calling set_no_show_all() is for preventing the item becomes
> visible
> > again, when user hide and then reshow the panel.
> 
> Well, I suspect that it was necessary because the original code resets
> "no_show_all" when showing:
> 
> if self._prop.visible:
>     self.set_no_show_all(False)
>     self.show_all()
> 
> because the "no_show_all" property affects self.show_all().  However, the new
> code does not reset the property when showing, and self.show() works even if
> "no_show_all" is True.

Actually self.set_no_show_all(True) is not for preventing re-show the item when
self.show_all() is called. It is for preventing re-showing the item, when
show_all() of the language bar (It is the parent widget of all property items )
is called.

> 
> My test case is, with ibus-m17n and the patch to hide status button if status
==
> title:
> 
> https://github.com/ueno/ibus-m17n/commit/390a2c7f
> 
> If I enable m17n anthy, type "a" and space, I see "漢" as the status prop. 
Then,
> when I type enter, I see the status prop is hidden, and Ctrl+Space twice to
> hide/show the panel, the status prop is still hidden.

Probably it is because m17n engine will call register_properties during disable
or enable engine events. Could you try 

Maybe you could try below steps when a property is hidden:
$ gconftool-2 --set --type=int /desktop/ibus/panel/show 0
# Hide language bar
$ gconftool-2 --set --type=int /desktop/ibus/panel/show 2
# Always show language bar

[Daiki Ueno, 2011-09-29 02:17:30]

On 2011/09/29 01:54:42, Peng wrote:
> Actually self.set_no_show_all(True) is not for preventing re-show the item
when
> self.show_all() is called. It is for preventing re-showing the item, when
> show_all() of the language bar (It is the parent widget of all property items
)
> is called.

Yeah, I know, but unintuitively, it does also affect self.show_all().

http://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n4061

> Maybe you could try below steps when a property is hidden:
> $ gconftool-2 --set --type=int /desktop/ibus/panel/show 0
> # Hide language bar
> $ gconftool-2 --set --type=int /desktop/ibus/panel/show 2
> # Always show language bar

I ran the following shell script on another terminal, enabled ibus-m17n and did
show/hide the status prop, the prop is still hidden after re-show.

#!/bin/sh
sleep 10
gconftool-2 --set --type=int /desktop/ibus/panel/show 0
sleep 5
gconftool-2 --set --type=int /desktop/ibus/panel/show 2

[Peng, 2011-09-29 02:53:45]

On 2011/09/29 02:17:30, Daiki Ueno wrote:
> On 2011/09/29 01:54:42, Peng wrote:
> > Actually self.set_no_show_all(True) is not for preventing re-show the item
> when
> > self.show_all() is called. It is for preventing re-showing the item, when
> > show_all() of the language bar (It is the parent widget of all property
items
> )
> > is called.
> 
> Yeah, I know, but unintuitively, it does also affect self.show_all().
> 
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n4061
> 
> > Maybe you could try below steps when a property is hidden:
> > $ gconftool-2 --set --type=int /desktop/ibus/panel/show 0
> > # Hide language bar
> > $ gconftool-2 --set --type=int /desktop/ibus/panel/show 2
> > # Always show language bar
> 
> I ran the following shell script on another terminal, enabled ibus-m17n and
did
> show/hide the status prop, the prop is still hidden after re-show.
> 
> #!/bin/sh
> sleep 10
> gconftool-2 --set --type=int /desktop/ibus/panel/show 0
> sleep 5
> gconftool-2 --set --type=int /desktop/ibus/panel/show 2

lgtm