Code review comment for lp:~cr3/checkbox/411526

Revision history for this message
Javier Collado (javier.collado) wrote :

I agree with the diff. Also the change from radio_button to checkbox_button
makes the code clearer. Thanks.

Marc Tardif wrote:
> I have just pushed the following diff to the trunk in response to your review, what do you think?
>
> === modified file 'checkbox_gtk/gtk_interface.py'
> --- checkbox_gtk/gtk_interface.py 2009-11-05 16:09:25 +0000
> +++ checkbox_gtk/gtk_interface.py 2009-11-06 16:07:25 +0000
> @@ -265,13 +265,14 @@
> option_table = {}
> vbox = self._get_widget("vbox_options_list")
> for option in options:
> - label = "_%s" % option.capitalize()
> - radio_button = gtk.CheckButton(label)
> - radio_button.show()
> - option_table[option] = radio_button
> - vbox.pack_start(radio_button, False, False, 0)
> + label = "_%s%s" % (option[0].upper(), option[1:])
> + check_button = gtk.CheckButton(label)
> + check_button.get_child().set_line_wrap(True)
> + check_button.show()
> + option_table[option] = check_button
> + vbox.pack_start(check_button, False, False, 0)
> if option in default:
> - radio_button.set_active(True)
> + check_button.set_active(True)
>
> self._set_hyper_text_view("hyper_text_view_options", text)
>
> @@ -296,8 +297,9 @@
> option_table = {}
> vbox = self._get_widget("vbox_options_list")
> for option in options:
> - label = "_%s" % option.capitalize()
> + label = "_%s%s" % (option[0].upper(), option[1:])
> radio_button = gtk.RadioButton(option_group, label)
> + radio_button.get_child().set_line_wrap(True)
> radio_button.show()
> option_table[option] = radio_button
> vbox.pack_start(radio_button, False, False, 0)
>
> === modified file 'gtk/checkbox-gtk.glade'
> --- gtk/checkbox-gtk.glade 2009-11-05 16:09:25 +0000
> +++ gtk/checkbox-gtk.glade 2009-11-06 16:08:04 +0000
> @@ -211,7 +211,7 @@
> </widget>
> <packing>
> <property name="padding">0</property>
> - <property name="expand">True</property>
> + <property name="expand">False</property>
> <property name="fill">True</property>
> </packing>
> </child>
>

« Back to merge proposal