GNOME Bugzilla – Bug 678551
improve calltips and resolve its dependencies from cpp-java plugin
Last modified: 2012-07-07 08:06:10 UTC
Created attachment 216940 [details] [review] improve calltips and resolve its dependencies from cpp-java plugin I ported the settings widget from the cpp-java plugin to the vala plugin. So the vala plugin is no longer depends on cpp-java plugin. Additionally to that the calltips show now the return type and the function name.
Review of attachment 216940 [details] [review]: Looks good overall, but it's better to split it into three patches: one for the preferences, one for the calltips, and one for the cpp-java. Some comments follow: ::: plugins/language-support-cpp-java/anjuta-language-cpp-java.plugin.in @@ -9,3 @@ [Language Support] -Languages=C,C++,Java,Vala -MimeTypes=text/x-c,text/x-java-source,text/x-vala I think this should have been part of the split of the indenter part (and btw, you forgot to remove IAnjutaIndenter from the interfaces above) ::: plugins/language-support-vala/Makefile.am @@ +54,2 @@ libanjuta_language_vala_la_vala.stamp: $(libanjuta_language_vala_la_VALASOURCES) + valac $(VALAFLAGS) --vapidir $(srcdir) --pkg $(LIBVALA) --pkg libanjuta-3.0 --pkg gtk+-3.0 --pkg config -C $^ it's better to have config.vapi as a source rather than a package. ::: plugins/language-support-vala/provider.vala @@ +142,3 @@ parameters = ((Vala.Method) sym).get_parameters (); + calltip.append (((Vala.Method) sym).return_type.to_qualified_string() + " "); + calltip.append (((Vala.Method) sym).name); It may be better to show the fully qualified name (or at least the class name) instead of just the method name (which the user has just typed anyway). What do you think? @@ +153,3 @@ if (var_type is Vala.DelegateType) { parameters = ((Vala.DelegateType) var_type).delegate_symbol.get_parameters (); + calltip.append (((Vala.Variable) sym).variable_type.to_qualified_string() + " "); you can just use var_type defined above instead of ((Vala.Variable) sym).variable_type
Created attachment 217026 [details] [review] resolve dependencies from cpp-java plugin > I think this should have been part of the split of the indenter part (and btw, > you forgot to remove IAnjutaIndenter from the interfaces above) see https://bugzilla.gnome.org/show_bug.cgi?id=676084#c6 > ::: plugins/language-support-vala/provider.vala > @@ +142,3 @@ > parameters = ((Vala.Method) sym).get_parameters (); > + calltip.append (((Vala.Method) > sym).return_type.to_qualified_string() + " "); > + calltip.append (((Vala.Method) sym).name); > > It may be better to show the fully qualified name (or at least the class name) > instead of just the method name (which the user has just typed anyway). What do > you think? Good idea. I couldn't find a method with more than one layer of parent class, so I could only test the new code on methods with one layer of parent class. :-(
Created attachment 217027 [details] [review] improve calltips in vala plugin
Review of attachment 217026 [details] [review]: Pushed, thanks.
Review of attachment 217027 [details] [review]: Looks good, I have a few comments (I've already fixed these, you don't need to do anything), and a couple questions. ::: plugins/language-support-vala/provider.vala @@ +143,3 @@ + calltip.append (((Vala.Method) sym).return_type.to_qualified_string() + " "); + var method_parents = new StringBuilder (); + var parent = ((Vala.Method) sym).parent_symbol; parent_symbol is a property of Vala.Symbol, you don't need to cast it to Vala.Method to use it. @@ +144,3 @@ + var method_parents = new StringBuilder (); + var parent = ((Vala.Method) sym).parent_symbol; + while (parent is Vala.Class) { I'm not sure if we want to treat classes separately form namespaces. In fact, being an inner class in vala isn't really different from being in a namespace. I think we should just use the fully qualified name. What do you think? @@ +149,3 @@ + } + calltip.append (method_parents.str); + calltip.append (((Vala.Method) sym).name); Same here, name is a property of Symbol. @@ +161,3 @@ parameters = ((Vala.DelegateType) var_type).delegate_symbol.get_parameters (); + calltip.append (var_type.to_qualified_string() + " "); + calltip.append (((Vala.Variable) sym).name); I'm not sure we want both the type and the name of the delegate, perhaps the type is enough. What do you think? Otherwise, the name would be added in all branches, and it would be better to add it at the end.
(In reply to comment #5) > @@ +144,3 @@ > + var method_parents = new StringBuilder (); > + var parent = ((Vala.Method) sym).parent_symbol; > + while (parent is Vala.Class) { > > I'm not sure if we want to treat classes separately form namespaces. In fact, > being an inner class in vala isn't really different from being in a namespace. > I think we should just use the fully qualified name. What do you think? I thought that it is what you mean. > @@ +161,3 @@ > parameters = ((Vala.DelegateType) > var_type).delegate_symbol.get_parameters (); > + calltip.append (var_type.to_qualified_string() + " "); > + calltip.append (((Vala.Variable) sym).name); > > I'm not sure we want both the type and the name of the delegate, perhaps the > type is enough. What do you think? > > Otherwise, the name would be added in all branches, and it would be better to > add it at the end. In Eclipse you have both. And if you describe the methods like javadoc, you have to say, what each parameter do. I think without a description the type is enough. In the future if we have a description text, the parameter name will help us to difference.
(In reply to comment #6) > (In reply to comment #5) > > @@ +144,3 @@ > > + var method_parents = new StringBuilder (); > > + var parent = ((Vala.Method) sym).parent_symbol; > > + while (parent is Vala.Class) { > > > > I'm not sure if we want to treat classes separately form namespaces. In fact, > > being an inner class in vala isn't really different from being in a namespace. > > I think we should just use the fully qualified name. What do you think? > I thought that it is what you mean. Well, I don't know which is better: including only the class name (may be ambiguous), or the fully qualified name (may be too long). But treating inner classes differently from normal classes doesn't seem very intersting to me. > In Eclipse you have both. And if you describe the methods like javadoc, you > have to say, what each parameter do. I believe there's a misunderstanding here, Java does not have delegates, and the Vala plugin for Eclipse doesn't work. So I believe there is no way Eclipse can do calltips for delegates. What I'm talking about is if you have a field of a delegate type (a delegate is like a function pointer in C), should it only print the delegate type, or also add the name. I've tried it, and the behavior of your patch is ambiguous. Let me give you an example so you can see what I'm talking about. str_hash is defined in glib-2.0.vapi as follows: [CCode (cname = "g_str_hash")] public static GLib.HashFunc<string> str_hash; but when it is called I get the following tooltip (with your patch) GLib.HashFunc<string> str_hash (K key) which looks like the str_hash would return a GLib.HashFunc<string> (which is wrong, it returns an uint). What I'm suggesting is to show only the type: GLib.HashFunc<string> (K key) (still a bit ugly because it's a generic delegate, we could probably make it better by finding the type of K, but that can wait). The disadvantage of this approach is that it doesn't show the return type. Another possibility is to have only the name of the delegate (along with the return type) uint str_hash (K key) (but that would make the problem with the generic delegate a bit more urgent). What do you think about all this? > I think without a description the type is enough. In the future if we have a > description text, the parameter name will help us to difference. Could you elaborate? Even if your idea may have nothing to do with my objections, it may be useful to implement in the future.
(In reply to comment #7) > > In Eclipse you have both. And if you describe the methods like javadoc, you > > have to say, what each parameter do. > > I believe there's a misunderstanding here, Java does not have delegates, and > the Vala plugin for Eclipse doesn't work. So I believe there is no way Eclipse > can do calltips for delegates. OK, there was a misunderstanding. I know Vala not very well... > What I'm talking about is if you have a field of a delegate type (a delegate is > like a function pointer in C), should it only print the delegate type, or also > add the name. I've tried it, and the behavior of your patch is ambiguous. Let > me give you an example so you can see what I'm talking about. str_hash is > defined in glib-2.0.vapi as follows: > > [CCode (cname = "g_str_hash")] > public static GLib.HashFunc<string> str_hash; > > but when it is called I get the following tooltip (with your patch) > > GLib.HashFunc<string> str_hash (K key) > > which looks like the str_hash would return a GLib.HashFunc<string> (which is > wrong, it returns an uint). What I'm suggesting is to show only the type: > > GLib.HashFunc<string> (K key) > > (still a bit ugly because it's a generic delegate, we could probably make it > better by finding the type of K, but that can wait). The disadvantage of this > approach is that it doesn't show the return type. > Another possibility is to have only the name of the delegate (along with the > return type) > > uint str_hash (K key) > > (but that would make the problem with the generic delegate a bit more urgent). > > What do you think about all this? I find the second one looks better: uint str_hash (K key) In this case delegates are treated like methods. Must the Vala programmer know if this is a delegate? If so, we have to mark this as a delegate. How can I provoke the calltip for str_hash? I only get a calltip, when I tip a attribute or method, which is a member of this class. So I was incapable to get a calltip for str_hash. What am I doing wrong? > > I think without a description the type is enough. In the future if we have a > > description text, the parameter name will help us to difference. > > Could you elaborate? Even if your idea may have nothing to do with my > objections, it may be useful to implement in the future. Without a description text, we have only a limited profit with parameter names of a method. bool validate (ssize_t max_len, char* end) With "max_len" and "end" the programmer can guess, what this parameters do. If we have a description text, we have to mark the parameters, when we explain the method. For example without the parameter names and so only with the types bool validate (ssize_t, out char*) we can only explain the method by saying "the first parameter", "the second parameter" etc. With parameter names, we can say what we really mean with the reference "max_len" and "end" (like javadoc: http://www.valadoc.org/#!api=glib-2.0/string.validate)
> uint str_hash (K key) > > In this case delegates are treated like methods. Must the Vala programmer know > if this is a delegate? If so, we have to mark this as a delegate. Not necessarily, if the programmer is going to call it, then it is like a method, > How can I provoke the calltip for str_hash? I only get a calltip, when I tip a > attribute or method, which is a member of this class. So I was incapable to get > a calltip for str_hash. What am I doing wrong? I don't know, works fine for me in a newly created vala project. > Without a description text, we have only a limited profit with parameter names > of a method. I thought about doing something like this, but don't have much time for it now. > bool validate (ssize_t, out char*) FWIW, this is invalid syntax for vala, so that shouldn't be a problem. Anyway, thanks for your patch, I've pushed it with the modifications we discussed.