Re: [ruby-gnome2-devel-ja] [patch] GtkBindingSet

[Kenichi K. <ko...@ma...>]

小宮です。

> もしよろしければコメント吸収版のパッチを
> もう一度投げていただけないでしょうか。

了解しました。後程パッチをお送りします。

> ＃すみません、ちと風邪気味＆デスマーチ気味で....。

お大事に＆お疲れさまです。私の方は急ぎませんのでお時間のある時に見てい
ただければ結構です。

> なるほど。
> ただ、newをオーバーライドするとGtk::BindingSetのサブクラスを作るのが
> 難しくなってしまうので、できればinitializeのままで行きたいところですね。
> 
> そこで、申し訳ないのですが、以下の２つを確認してみていただけないで
> しょうか。
> 
> (案１)
> rbgobj_boxed_not_copy_obj(GTK_TYPE_BINDING_SET);
> をInit_gtk_bindings()の先頭あたりで宣言する。

既に gtk_bindingset_get_type()が最初に呼ばれた時にこれをするようにして
ますが、rbgobj_boxed_initialize()はフラグを無視してコピーするのでうま
く行きませんでした。newをオーバーライドしたのは
rbgobj_boxed_initialize()ではなく、rbgobj_make_boxed()でインスタンスを
生成したかったからです。

> 
> (案２)
> gtk_bindingset_copy(const GtkBindingSet* bin)の方をいじる。
> 
> static GtkBindingSet*
> gtk_bindingset_copy(const GtkBindingSet* bin)
> {
>   /* not copy here */
>   return bin;
> }
> 
> 一応、案１＞案２の優先度です。どちらも上記以外は元(initialize)のままです。

元のままというのはCVSのまま、つまり

    RBGTK_INITIALIZE(self, gtk_binding_set_new(RVAL2CSTR(set_name)));

を呼ぶという意味でうしょうか？このままだとクラッシュしてたので G_INITIALIZE
を呼ぶように変えてみたのですが…

> 
> ＃どちらも、逆にコピーが必要そうな場所があったら明示的に
> ＃g_newしないといけない場所が出てくるかもしれません。

GtkBindingSet GTK側でハッシュに登録して管理してるのでコピーしてしまう
と動作がおかしくなるように思います。(最初コピーされていたので 
add_signal で登録されたエントリーをactivateした際クラッシュしてました。)

> >   * Gtk::BindingSet.by_class
> >     gtk_binding_set_by_class()。 Gtk::Object.binding_set の方が
> >     良いでしょうか？
> 
> 確かにそうですね。
> しかも、使用目的を考えると、引数を省略可能にして以下のようにも
> 書けたりすると良さげですがどうでしょう。
> ＃書けるのかな(^^;)。
> 
> class MyTreeView < Gtk::TreeView
>   bset = binding_set
>   bset.add_signal(Gdk::Keyval::GDK_space, 0, "move_cursor", 
>                   Gtk::MOVEMENT_PAGES, 1, false)
> 
>   def initialize
>      :
>   end
> end

ではこうします。もともとこう書けた方が良いかな、とは思ったのですが本来
の Gtk と違うクラスにメソッドを持っていってしまうのがアリなのか分から
なかったので見送りました。

> 
> >   * Gtk::BindingSet.activate
> >     gtk_bindings_activate()。Gtk::Object#activate_binding とか
> >     の方が良いでしょうか？
> 
> Gtk::Object#bindings_activateというのがあるのでそちらを使うと
> よろしいかと。

見落してました。これは消します。

> 
> >   * Gtk::BindingSet#add_signal
> >     gtk_binding_entry_add_signal()。
> > 
> >     シグナルに渡せるパラメータは Float, 整数、文字列のみ。to_int 
> >     と to_str の順で変換を試みる。true/false は 1/0 に変換される。
> 
> > +            arg->d.string_data = StringValueCStr(param);
> 
> は念のためRVAL2CSTRを使ってください。

RVAL2CSTRだとStringValuePtr相当みたいですが、渡した文字列は内部で 
g_strdup されるので StringValueCStr したいところです。

> 
> > +        } else if (rb_respond_to (param, rb_intern("to_str"))) {
> > +        } else if (rb_respond_to (param, rb_intern("to_int"))) {
> 
> は微妙ですねぇ(^^;)。
> to_intの方は、GLib::EnumかGLib::FlagsかInteger型の時、って限定
> した方が良さそうな気がしますがどうでしょうか。
> to_strの方は、極端な話to_sにしちゃってif文の優先度を下げるようにするとか。

Ruby 的に整数、文字列が期待されている文脈では to_int, to_str を呼ぶと
いう習慣なのかなと思ってこうしてみたのですが…

優先順位は逆にしても良いと思いますが、to_s を暗黙の変換に使うのは本来
の使用目的からはずれると思うので、to_str しないなら何もしない方が良い
と思いますがどうでしょうか？

> 
> true/falseのところはGTK+のソースを読むと、G_TYPE_LONGではなく
> G_TYPE_BOOLEANを明示的に使っているところが多いのですが、
> こちらもそうした方が良いのではないでしょうか。

ここは G_TYPE_LONG でないとまずいんです。
gtk_bindings_entry_add_singall() の中では G_TYPE_LONG, G_TYPE_DOUBLE,
G_TYPE_STRING の3通りにしか対応していないので。

gtk_bindings_entry_add_singal() (lがないやつ)の方でも G_TYPE_BOOLEAN を 
G_TYPE_LONG に変換して l つきのやつに渡してます。

> あと、サンプルコードの方も頂いちゃいたいのですがよろしいでしょうか？

はい。

----
小宮 健一