Issue 5956045: Enhance ibus-ui-gtk3 switcher labels. (Closed)
|
Created:
12 years, 3 months ago by fujiwara Modified:
12 years ago Reviewers:
shawn.p.huang CC:
shawn.p.huang_gmail.com Base URL:
git@github.com:ibus/ibus.git@master Visibility:
Public. |
DescriptionEnhance ibus-ui-gtk3 switcher labels.
TEST=Linux desktop
Patch Set 1 #
Total comments: 1
Patch Set 2 : Updated with message #4. #
Total comments: 1
Patch Set 3 : Updated with message #7. #
Total comments: 1
Patch Set 4 : Updated with message #9. #Patch Set 5 : Fixed a bug. #Patch Set 6 : Updated for arrow keys. #
Total comments: 2
Patch Set 7 : Updated with message #16. #Patch Set 8 : Updated with the latest master. #MessagesTotal messages: 18
https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala#newcode125 ui/gtk3/panel.vala:125: Gtk.Settings.get_default().get_property("gtk-font-name", ref value); I remember in previous custom font CL, I suggest not set font, if font_name is NULL. Why you revert this change? any issue?
Sign in to reply to this message.
On 2012/03/29 14:34:31, Peng wrote: > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala > File ui/gtk3/panel.vala (right): > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala#newcode125 > ui/gtk3/panel.vala:125: Gtk.Settings.get_default().get_property("gtk-font-name", > ref value); > I remember in previous custom font CL, I suggest not set font, if font_name is > NULL. Why you revert this change? any issue? Yeah, I have one issue. If I need the default font in the previous custom font, I can just remove Gtk.CssProvider. But I need the default font size in this patch because I set the icon size = the default font size x 4. If "Use custom font" is disabled in ibup-setup GUI, I think the theme's font size is needed because I have no idea to get the font size x 4 without this way.
Sign in to reply to this message.
On 2012/03/30 01:29:02, fujiwara wrote: > On 2012/03/29 14:34:31, Peng wrote: > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala > > File ui/gtk3/panel.vala (right): > > > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala#newcode125 > > ui/gtk3/panel.vala:125: > Gtk.Settings.get_default().get_property("gtk-font-name", > > ref value); > > I remember in previous custom font CL, I suggest not set font, if font_name is > > NULL. Why you revert this change? any issue? > > Yeah, I have one issue. > If I need the default font in the previous custom font, I can just remove > Gtk.CssProvider. > > But I need the default font size in this patch because I set the icon size = the > default font size x 4. > If "Use custom font" is disabled in ibup-setup GUI, I think the theme's font > size is needed because I have no idea to get the font size x 4 without this way. I see. But this way may has other problem. Like, if the system font is changed, the panel will still using the old font. I think for the icon, probably we should also respect the system settings, we user choices to use system. Settings.
Sign in to reply to this message.
On 2012/03/30 01:34:08, Peng wrote: > On 2012/03/30 01:29:02, fujiwara wrote: > > On 2012/03/29 14:34:31, Peng wrote: > > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala > > > File ui/gtk3/panel.vala (right): > > > > > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala#newcode125 > > > ui/gtk3/panel.vala:125: > > Gtk.Settings.get_default().get_property("gtk-font-name", > > > ref value); > > > I remember in previous custom font CL, I suggest not set font, if font_name > is > > > NULL. Why you revert this change? any issue? > > > > Yeah, I have one issue. > > If I need the default font in the previous custom font, I can just remove > > Gtk.CssProvider. > > > > But I need the default font size in this patch because I set the icon size = > the > > default font size x 4. > > If "Use custom font" is disabled in ibup-setup GUI, I think the theme's font > > size is needed because I have no idea to get the font size x 4 without this > way. > I see. But this way may has other problem. Like, if the system font is changed, > the panel will still using the old font. > > I think for the icon, probably we should also respect the system settings, we > user choices to use system. Settings. OK, I see. I will get the font family besides the font size.
Sign in to reply to this message.
On 2012/03/30 01:38:01, fujiwara wrote: > On 2012/03/30 01:34:08, Peng wrote: > > On 2012/03/30 01:29:02, fujiwara wrote: > > > On 2012/03/29 14:34:31, Peng wrote: > > > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala > > > > File ui/gtk3/panel.vala (right): > > > > > > > > > https://codereview.appspot.com/5956045/diff/1/ui/gtk3/panel.vala#newcode125 > > > > ui/gtk3/panel.vala:125: > > > Gtk.Settings.get_default().get_property("gtk-font-name", > > > > ref value); > > > > I remember in previous custom font CL, I suggest not set font, if > font_name > > is > > > > NULL. Why you revert this change? any issue? > > > > > > Yeah, I have one issue. > > > If I need the default font in the previous custom font, I can just remove > > > Gtk.CssProvider. > > > > > > But I need the default font size in this patch because I set the icon size = > > the > > > default font size x 4. > > > If "Use custom font" is disabled in ibup-setup GUI, I think the theme's font > > > size is needed because I have no idea to get the font size x 4 without this > > way. > > I see. But this way may has other problem. Like, if the system font is > changed, > > the panel will still using the old font. > > > > I think for the icon, probably we should also respect the system settings, we > > user choices to use system. Settings. > > OK, I see. I will get the font family besides the font size. Revised the patch.
Sign in to reply to this message.
http://codereview.appspot.com/5956045/diff/8001/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): http://codereview.appspot.com/5956045/diff/8001/ui/gtk3/panel.vala#newcode79 ui/gtk3/panel.vala:79: m_desktop_interface.changed.connect(desktop_interface_changed_cb); Is it necessary to listen on the change of desktop interface? I thought the font will be changed automatically, when users change the font settings of desktop.
Sign in to reply to this message.
On 2012/04/11 01:36:02, Peng wrote: > http://codereview.appspot.com/5956045/diff/8001/ui/gtk3/panel.vala > File ui/gtk3/panel.vala (right): > > http://codereview.appspot.com/5956045/diff/8001/ui/gtk3/panel.vala#newcode79 > ui/gtk3/panel.vala:79: > m_desktop_interface.changed.connect(desktop_interface_changed_cb); > Is it necessary to listen on the change of desktop interface? I thought the font > will be changed automatically, when users change the font settings of desktop. The previous patch icon size is the default font size x 4 so it would not be get the size automatically but need to calculate the size. Now I think the 4 times is not useful for users so I added a new dconf value custom_icon_font for the customization. If use_custom_font is disabled, switcher uses the default size 34.
Sign in to reply to this message.
http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in File data/ibus.schemas.in (right): http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 data/ibus.schemas.in:228: <key>/schemas/desktop/ibus/panel/custom_icon_font</key> I think it is not necessary to add this config value. Just one custom font should be enough. And I suggest to do some research on gnome how to handle icon size when font is changed. We should follow it, right? For example, if you change the font of an application, how gnome changes the icons size in menu.
Sign in to reply to this message.
On 2012/04/11 13:36:58, Peng wrote: > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in > File data/ibus.schemas.in (right): > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 > data/ibus.schemas.in:228: > <key>/schemas/desktop/ibus/panel/custom_icon_font</key> > I think it is not necessary to add this config value. Just one custom font > should be enough. > > And I suggest to do some research on gnome how to handle icon size when font is > changed. We should follow it, right? > > For example, if you change the font of an application, how gnome changes the > icons size in menu. OK, I set the hard-coded icon size at the moment.
Sign in to reply to this message.
On 2012/04/12 06:04:11, fujiwara wrote: > On 2012/04/11 13:36:58, Peng wrote: > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in > > File data/ibus.schemas.in (right): > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 > > data/ibus.schemas.in:228: > > <key>/schemas/desktop/ibus/panel/custom_icon_font</key> > > I think it is not necessary to add this config value. Just one custom font > > should be enough. > > > > And I suggest to do some research on gnome how to handle icon size when font > is > > changed. We should follow it, right? > > > > For example, if you change the font of an application, how gnome changes the > > icons size in menu. > > OK, I set the hard-coded icon size at the moment. I think this CL will change the switch popup dialog looking. It is better to have UI design and discuss it first. And I also think the ibus default panel, it is for all desktop environments. Maybe it is better to keep it simple. Will fedora ship with gnome3 ibus panel by as default panel? I think it is good to make gnome3 ibus panel fancy. What do you think?
Sign in to reply to this message.
On 2012/04/12 16:36:53, Peng wrote: > On 2012/04/12 06:04:11, fujiwara wrote: > > On 2012/04/11 13:36:58, Peng wrote: > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in > > > File data/ibus.schemas.in (right): > > > > > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 > > > data/ibus.schemas.in:228: > > > <key>/schemas/desktop/ibus/panel/custom_icon_font</key> > > > I think it is not necessary to add this config value. Just one custom font > > > should be enough. > > > > > > And I suggest to do some research on gnome how to handle icon size when font > > is > > > changed. We should follow it, right? > > > > > > For example, if you change the font of an application, how gnome changes the > > > icons size in menu. > > > > OK, I set the hard-coded icon size at the moment. > > I think this CL will change the switch popup dialog looking. It is better to > have UI design and discuss it first. > > And I also think the ibus default panel, it is for all desktop environments. > Maybe it is better to keep it simple. Will fedora ship with gnome3 ibus panel by > as default panel? I think it is good to make gnome3 ibus panel fancy. > > What do you think? The problem is that the language id is more important that the IME name. I'm not sure why you think this is not simple.
Sign in to reply to this message.
On 2012/04/13 01:25:04, fujiwara wrote: > On 2012/04/12 16:36:53, Peng wrote: > > On 2012/04/12 06:04:11, fujiwara wrote: > > > On 2012/04/11 13:36:58, Peng wrote: > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in > > > > File data/ibus.schemas.in (right): > > > > > > > > > > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 > > > > data/ibus.schemas.in:228: > > > > <key>/schemas/desktop/ibus/panel/custom_icon_font</key> > > > > I think it is not necessary to add this config value. Just one custom font > > > > should be enough. > > > > > > > > And I suggest to do some research on gnome how to handle icon size when > font > > > is > > > > changed. We should follow it, right? > > > > > > > > For example, if you change the font of an application, how gnome changes > the > > > > icons size in menu. > > > > > > OK, I set the hard-coded icon size at the moment. > > > > I think this CL will change the switch popup dialog looking. It is better to > > have UI design and discuss it first. > > > > And I also think the ibus default panel, it is for all desktop environments. > > Maybe it is better to keep it simple. Will fedora ship with gnome3 ibus panel > by > > as default panel? I think it is good to make gnome3 ibus panel fancy. > > > > What do you think? > > The problem is that the language id is more important that the IME name. > I'm not sure why you think this is not simple. It is a feeling from your patch. We did not discuss the UI design first. I don't know what's the new looking after your patch. It is hard to review for the code.
Sign in to reply to this message.
On 2012/04/13 03:48:18, Peng wrote: > On 2012/04/13 01:25:04, fujiwara wrote: > > On 2012/04/12 16:36:53, Peng wrote: > > > On 2012/04/12 06:04:11, fujiwara wrote: > > > > On 2012/04/11 13:36:58, Peng wrote: > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in > > > > > File data/ibus.schemas.in (right): > > > > > > > > > > > > > > > > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 > > > > > data/ibus.schemas.in:228: > > > > > <key>/schemas/desktop/ibus/panel/custom_icon_font</key> > > > > > I think it is not necessary to add this config value. Just one custom > font > > > > > should be enough. > > > > > > > > > > And I suggest to do some research on gnome how to handle icon size when > > font > > > > is > > > > > changed. We should follow it, right? > > > > > > > > > > For example, if you change the font of an application, how gnome changes > > the > > > > > icons size in menu. > > > > > > > > OK, I set the hard-coded icon size at the moment. > > > > > > I think this CL will change the switch popup dialog looking. It is better to > > > have UI design and discuss it first. > > > > > > And I also think the ibus default panel, it is for all desktop environments. > > > Maybe it is better to keep it simple. Will fedora ship with gnome3 ibus > panel > > by > > > as default panel? I think it is good to make gnome3 ibus panel fancy. > > > > > > What do you think? > > > > The problem is that the language id is more important that the IME name. > > I'm not sure why you think this is not simple. > > It is a feeling from your patch. We did not discuss the UI design first. I don't > know what's the new looking after your patch. It is hard to review for the code. It enhances the alignment.
Sign in to reply to this message.
On 2012/04/13 03:48:18, Peng wrote: > On 2012/04/13 01:25:04, fujiwara wrote: > > On 2012/04/12 16:36:53, Peng wrote: > > > On 2012/04/12 06:04:11, fujiwara wrote: > > > > On 2012/04/11 13:36:58, Peng wrote: > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in > > > > > File data/ibus.schemas.in (right): > > > > > > > > > > > > > > > > > > > > http://codereview.appspot.com/5956045/diff/15001/data/ibus.schemas.in#newcode228 > > > > > data/ibus.schemas.in:228: > > > > > <key>/schemas/desktop/ibus/panel/custom_icon_font</key> > > > > > I think it is not necessary to add this config value. Just one custom > font > > > > > should be enough. > > > > > > > > > > And I suggest to do some research on gnome how to handle icon size when > > font > > > > is > > > > > changed. We should follow it, right? > > > > > > > > > > For example, if you change the font of an application, how gnome changes > > the > > > > > icons size in menu. > > > > > > > > OK, I set the hard-coded icon size at the moment. > > > > > > I think this CL will change the switch popup dialog looking. It is better to > > > have UI design and discuss it first. > > > > > > And I also think the ibus default panel, it is for all desktop environments. > > > Maybe it is better to keep it simple. Will fedora ship with gnome3 ibus > panel > > by > > > as default panel? I think it is good to make gnome3 ibus panel fancy. > > > > > > What do you think? > > > > The problem is that the language id is more important that the IME name. > > I'm not sure why you think this is not simple. > > It is a feeling from your patch. We did not discuss the UI design first. I don't > know what's the new looking after your patch. It is hard to review for the code. It enhances the alignment. BTW, did you try the ibus switcher on KDE? The switcher does not show the dashed frame line on Fedora 17 KDE and I think my patch also makes sense for that problem.
Sign in to reply to this message.
https://codereview.appspot.com/5956045/diff/30001/ui/gtk3/switcher.vala File ui/gtk3/switcher.vala (right): https://codereview.appspot.com/5956045/diff/30001/ui/gtk3/switcher.vala#newco... ui/gtk3/switcher.vala:27: using Atk; How about arrange them in alpha order? https://codereview.appspot.com/5956045/diff/30001/ui/gtk3/switcher.vala#newco... ui/gtk3/switcher.vala:162: private Gtk.Button ime_button_new(IBus.EngineDesc engine) { Could you make ime_button a class?
Sign in to reply to this message.
On 2012/05/21 11:03:47, Peng wrote: > https://codereview.appspot.com/5956045/diff/30001/ui/gtk3/switcher.vala > File ui/gtk3/switcher.vala (right): > > https://codereview.appspot.com/5956045/diff/30001/ui/gtk3/switcher.vala#newco... > ui/gtk3/switcher.vala:27: using Atk; > How about arrange them in alpha order? > > https://codereview.appspot.com/5956045/diff/30001/ui/gtk3/switcher.vala#newco... > ui/gtk3/switcher.vala:162: private Gtk.Button ime_button_new(IBus.EngineDesc > engine) { > Could you make ime_button a class? I updated this.
Sign in to reply to this message.
|
