GNOME Bugzilla – Bug 637774
Add Vala class wizard to class-gen
Last modified: 2010-12-27 22:55:38 UTC
Created attachment 176860 [details] [review] Add Vala class wizard to class-gen plugin The attached patch adds a Vala class wizard for the class-gen plugin. This is a GCI task [1] [1] http://www.google-melange.com/gci/task/show/google/gci2010/gnome/t129174915339 I have started to work off git HEAD so it will be compatible to the previous class wizard patches.
Review of attachment 176860 [details] [review]: As said before on IRC this patch doesn't apply on the UI file for me. Could you please create a patch that applied to current master. Thanks!
Created attachment 176941 [details] [review] Add Vala class wizard to class-gen plugin
Created attachment 176942 [details] [review] Update class-gen UI file
(In reply to comment #1) > Review of attachment 176860 [details] [review]: > > As said before on IRC this patch doesn't apply on the UI file for me. Could you > please create a patch that applied to current master. > > Thanks! OK, git apply should now really work with these patches. I have remade the patch by hand off the JavaScript wizard branch. I also tested it with git apply this time. I consciously made two patches, because I think the root of the apply error was the UI file. Known issues: The class scope is useless, because the selected model item doesn't get passed to the template. This also doesn't work with the C++ wizard, anymore. I have to check if this is due to my code or other code in Anjuta. The rest should just work.
Even though it still doesn't apply (I had to manually remove the first and last hunk of the second patch), and doesn't work for me (properties/signal names don't appear), I have some comments : * Instead of adding a checkbox for deriving from GLib.Object, you could put it in the base class field by default (and let the user change it) as it's done in the gobject class gen. * the scope for properties should be about getter/setter rather than the field which actually store its value (the latter should always be private IMO). * It would be nice to have automatic properties. * You're missing the 'internal' scope (it would also be nice to add some sort of implicit scope i.e. when you don't specify at all). Also the protected scope doesn't apply for classes AFAIK.
Review of attachment 176942 [details] [review]: The patch still didn't apply currently on the UI file but I fixed that manually. Would be nice if you could take care of Abderrahim comments! Thanks!
(In reply to comment #5) > Even though it still doesn't apply (I had to manually remove the first and last > hunk of the second patch), and doesn't work for me (properties/signal names > don't appear), I have some comments : > > * Instead of adding a checkbox for deriving from GLib.Object, you could put it > in the base class field by default (and let the user change it) as it's done in > the gobject class gen. Good idea! > * the scope for properties should be about getter/setter rather than the field > which actually store its value (the latter should always be private IMO). OK. > * It would be nice to have automatic properties. What do you mean by "automatic properties"? > * You're missing the 'internal' scope OK. > (it would also be nice to add some sort of implicit scope i.e. when you don't > specify at all). OK. > Also the protected scope doesn't apply for classes AFAIK. Yes, the class scope doesn't work for me either.
(In reply to comment #7) > > * It would be nice to have automatic properties. > > What do you mean by "automatic properties"? public int my_property {get; set;} i.e. using the default getter/setter, without explicitly defining a field for storing its value. (Here I should insert a link to the vala tutorial but I'm in a hurry) > > Also the protected scope doesn't apply for classes AFAIK. > > Yes, the class scope doesn't work for me either. What I meant is that a class cannot be protected, but I'm testing now and it seems to work. Anyway, I don't think it's useful to have a class that's neither public nor internal (except maybe for inner classes).
(In reply to comment #8) > (In reply to comment #7) > > > * It would be nice to have automatic properties. > > > > What do you mean by "automatic properties"? > > public int my_property {get; set;} > > i.e. using the default getter/setter, without explicitly defining a field for > storing its value. (Here I should insert a link to the vala tutorial but I'm in > a hurry) Thanks for pointing that out. I think in this case, I will replace the column for "Setter" with "Automatic". Or is this expression legal: public int my_property {get;} > > > Also the protected scope doesn't apply for classes AFAIK. > > > > Yes, the class scope doesn't work for me either. > > What I meant is that a class cannot be protected, but I'm testing now and it > seems to work. Anyway, I don't think it's useful to have a class that's neither > public nor internal (except maybe for inner classes). OK, I looked up the 'internal' scope, and I could only find examples for properties which can be internal. So are there are also 'internal' classes in Vala? The documentation lacks this information. Maybe we should remove the 'protected' scope?
> Or is this expression legal: > > public int my_property {get;} Nope, it's not.
(In reply to comment #8) > (In reply to comment #7) > > > * It would be nice to have automatic properties. > > > > What do you mean by "automatic properties"? > > public int my_property {get; set;} How I will implement that: public class Foo { public string name { get; set; } public Foo (string name) { this.name = name; } } Tell me if that is correct, please.
Yes, that seems fine to me. with options for choosing public/internal for classes, public/protected/internal for properties and whether to have manual getter/setter. Adding all the properties to the constructor may be nice, but I'm not sure it'd be the best thing to do. Anyway, I think you can implement it like this for now (and if some users complain, we'll change it ;-)) Another comment I forgot last time : Vala still doesn't support signals with return values, so it would be nice if the signal return value was prefilled with void.
(In reply to comment #12) > Yes, that seems fine to me. with options for choosing public/internal for > classes, public/protected/internal for properties and whether to have manual > getter/setter. Shouldn't we also leave 'private' scope. > > Adding all the properties to the constructor may be nice, but I'm not sure it'd > be the best thing to do. Anyway, I think you can implement it like this for now > (and if some users complain, we'll change it ;-)) I agree ;-) > Another comment I forgot last time : Vala still doesn't support signals with > return values, so it would be nice if the signal return value was prefilled > with void. Good to know, will fix that, too.
Created attachment 177104 [details] [review] Various fixed for vala wizard in class-gen Please read the commit details in the patch to see what changed.
Review of attachment 177104 [details] [review]: Thanks! That looks really good to me!