For D and Python, we try to read the calltip from the constructor method (e.g. "__init__" for Python) instead of from the class tag itself. If we don't find a matching constructor method tag, now we continue looking at the class tag itself.
This is useful for global tags where the constructor signature might be defined on the class tag itself. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3334
-- Commit Summary --
* Use constructor method calltip only if available
-- File Changes --
M src/editor.c (14)
-- Patch Links --
https://github.com/geany/geany/pull/3334.patch https://github.com/geany/geany/pull/3334.diff
Ah, OK, I'm going through the issues one by one and wasn't aware of this when I mentioned the related thing in #3039.
I think the code shouldn't hurt anything but I don't think real ctags (unlike the tag generation script) adds parameters to class tags and leaves them only in the constructor methods. So I think it would be better to rather simulate the ctags behavior in the script so this code wouldn't be needed.
@eht16 how is the constructor signature defined on the class not the `_init_()` function in Python?
The script uses the `inspect` python library and I think it does some "smart" things for you like using `__init__()` arguments for the class constructor.
Ahhh, I see, it fakes the constructor rather than Python having added a new ability to declare it. Fine.
I think the code shouldn't hurt anything but I don't think real ctags (unlike the tag generation script) adds parameters to class tags and leaves them only in the constructor methods. So I think it would be better to rather simulate the ctags behavior in the script so this code wouldn't be needed.
Hmm, this would add an `__init__` tag for each class in the generated tags file only to satify this special logic for Python (and D). Those additional `___init__` tags are of no other use for the user and just might cause confusion if you get suggestions and lots of additional calltips when writing own `__init__` methods.
Apart from the generated tag file use case, I'd consider this change to make this special logic a bit less hard because now the normal calltip selection process will continue as for all other languages. Before, it roughly stopped trying find a matching calltip if no constructor tag was found.
The script uses the `inspect` python library and I think it does some "smart" things for you like using `__init__()` arguments for the class constructor.
It's actually the `inspect` library which does "smart" things. The script just asks `inspect` to get the signature for a class object. I would not call this "fake" :).
Hmm, this would add an __init__ tag for each class in the generated tags file only to satify this special logic for Python (and D).
True, but this is how the command-line ctags works. And it also works this way for user-written code. So for ``` MyClass( ``` you get calltip containing ``` __init__(...) ``` and not ``` MyClass(...) ``` Adding arguments to classes would make calltips look different for user-written code and global tags which I think is confusing.
Those additional ___init__ tags are of no other use for the user and just might cause confusion if you get suggestions and lots of additional calltips when writing own __init__ methods.
You get only the `__init__()` from the class whose constructor you call for the calltips, `__init__()` functions from other classes are not shown.
Apart from the generated tag file use case, I'd consider this change to make this special logic a bit less hard because now the normal calltip selection process will continue as for all other languages. Before, it roughly stopped trying find a matching calltip if no constructor tag was found.
Yes, but only for `class` and `struct` types which don't have arguments (at least the way `ctags` works) so the following code won't find any arguments. Again, this patch won't hurt anything and can be merged, I just think the script (if it's not a big problem) should detect this special case and make the output similar to `ctags` behavior.
It's actually the inspect library which does "smart" things. The script just asks inspect to get the signature for a class object. I would not call this "fake" :).
It's "smart" compared to `ctags` because the way `ctags` parses code, the moment it encounters `def MyClass:` it doesn't know the `__init__()` so it emits (and will always emit) `MyClass` without arguments.
I would not call this "fake" :).
"Fake" in as much as its not something written in the source, perhaps call it "synthetic" then :-)
Hmm, this would add an **init** tag for each class in the generated tags file only to satify this special logic for Python (and D).
True, but this is how the command-line ctags works. And it also works this way for user-written code. So for
MyClass(you get calltip containing
__init__(...)and not
MyClass(...)Adding arguments to classes would make calltips look different for user-written code and global tags which I think is confusing.
Yes. I didn't even realize the difference but yes, other users might notice and be confused.
Those additional _**init** tags are of no other use for the user and just might cause confusion if you get suggestions and lots of additional calltips when writing own **init** methods.
You get only the `__init__()` from the class whose constructor you call for the calltips, `__init__()` functions from other classes are not shown.
When requesting the calltip for the class itself, yes. If you are requesting the calltip for an `__init__` method, all signatures of all known `__init__` are shown and this is what I were referring to.
I tried to save adding the the `__init__` tags to the generated tags file which seemed unnecessary. But I guess it's also ok to add them there and so have a generated tags file which is very close to what ctags would generate. Yet, it will add about 1500 additional tags and the file size of the Python standard library tags file increases from 1.2 mb to 1.3 mb.
I'm not that much against your suggestion to add the `__init__` methods to the generated tags file and drop this PR therefore. I just wanted to point out why I did it the other way initially.
Adding arguments to classes would make calltips look different for user-written code and global tags which I think is confusing.
Yes. I didn't even realize the difference but yes, other users might notice and be confused.
On the other hand we could change the calltip code to show the class name instead of `__init__` (and attach the arguments from `__init__` to it) which would make it consistent for users the other way round.
When requesting the calltip for the class itself, yes. If you are requesting the calltip for an __init__ method, all signatures of all known __init__ are shown and this is what I were referring to.
OK, I didn't think about this and yes, I can imagine it could get quite annoying (plus possibly slow) seeing calltips when writing your own `__init__` functions (I don't think invoking `__init__()` directly is common in Python).
So yeah, I'm now leaning towards dropping `__init__` from the tag file, applying this patch and also modifying the calltip code to show the class name instead of `__init__`. What do you think?
@techee
OK, I didn't think about this and yes, I can imagine it could get quite annoying (plus possibly slow) seeing calltips when writing your own __init__ functions (I don't think invoking __init__() directly is common in Python).
`__init__()` needs to be manually called on super classes inside derived class `__init__()`s, but thats the only common case of explicit call AFAIK.
But if the tooltip on typing `__init__(` either as definition or call was a list of all the `__init__()` functions in all global tags it will be pretty much useless anyway, so its not much of a loss.
So yeah, I'm now leaning towards dropping __init__ from the tag file, applying this patch and also modifying the calltip code to show the class name instead of __init__. What do you think?
Well, the tooltip is supposed to be helpful, so showing the list of `__init__()`s as a list of constructors looks like what the user should type, but more important is being consistent, both global tags classes and runtime parsed classes should show the same.
The symbols in the sidebar will still show `__init__()`s for open files, so they are not lost.
So would agree.
With this PR class tooltips would look like:
  
also modifying the calltip code to show the class name instead of `__init__`.
after this it would look like: 
Great idea. The other variants keep the same. If you agree, I'd commit that change.
There is still a difference: signatures created by ctags (also within Geany) list the "self" argument as part of the signature while it is missing from the generated tags file. Adding it to the generated tags file would be easy (probably). But this raised the question if "self" should maybe dropped from the calltip for Python at all. This is another very Python specific thing: each method must accept a first argument, called "self" by convention, but this argument is never passed when calling the method. So, the signature should contain the special argument while a calltip probably should omit it as you must not pass it when calling the method.
On the other hand, this would require even more Python specific code :(. What do you guys think?
If you agree, I'd commit that change.
Sounds good.
But this raised the question if "self" should maybe dropped from the calltip for Python at all. ... On the other hand, this would require even more Python specific code :(.
This is the hard one - I don't like the `self` in the constructor tooltip but I don't like something too language-specific to TM either. I tried to move all the language-specific-selection logic to the various `tm_parser.c` functions but this one is so specific to python that it should probably get implemented directly without this callback. Anyway, I don't have a strong opinion whether to do it or not.
So, the signature should contain the special argument while a calltip probably should omit it as you must not pass it when calling the method. On the other hand, this would require even more Python specific code :(.
Pyeany? Oh wait its not April 1 ;-P
Agree with @techee that sprinkling language specific code throughout is nasty, it makes using things like LSPs harder. But anyway it should be a separate PR, and maybe it could start a "language_specific.c" file and put it in a function there, and other language specific parts of Geany could be migrated over (a long) time.
But on this PR, the last example replacing the first and the others the same looks great to me.
But anyway it should be a separate PR, and maybe it could start a "filetype_specific.c" file and put it in a function there, and other language specific parts of Geany could be migrated over (a long) time.
Just for the info, we have this `filetype_specific.c` in TM now and it's called `tm_parser.c`. The only exception where it isn't used is this check
https://github.com/geany/geany/blob/96342f3eb94dc6d49a05a65c9d73becca9009aa0...
which is kind of constant so I thought it wouldn't be necessary to add a callback to `tm_parser.c` for this, the rest of TM doesn't contain any language-specific code.
I'm kind of proud I set an example here for the rest of the code and that in this area TM is is now better than the rest of Geany at least in something :-). These would be the the screenshots of "the most salient lines of code" for the last year if some [idiot](https://arstechnica.com/tech-policy/2022/11/musk-emails-remaining-twitter-st...) asked me to do something like that (not really, I'd be gone).
Just for the info, we have this filetype_specific.c in TM now and it's called tm_parser.c.
Whilst TM may be the main part, I was thinking about _all_ the places where there are "if Python/C/C++" parts eg `editor.c`, not just TM related. Once its all in one file it becomes easier to add "if using LSP else" to the front of the filetype tests, and/or hive them off to filetype specific plugins. Just needs "somebody" to do it.
I'm kind of proud I set an example here for the rest of the code
A _good_ example in fact, neat :-)
and that in this area TM is is now better than the rest of Geany at least in something :-).
Still nobody (but you) wants to touch it ;-P
Still maybe the cleaner can do it (reference to your last para).
hello every one ... i need shall
hello
@bdfighter2p if you have something to say that is relevant to this topic, say it. If it is not relevant to this topic please raise a new issue, or if you want to introduce yourself please start a new discussion.
The changes below replace the `__init__` method name on the calltip with its class and so make the calltip look more consistent with the others, as shown in https://github.com/geany/geany/pull/3334#issuecomment-1321212468.
```diff diff --git a/src/editor.c b/src/editor.c index 20b9c56cb..a21aa8667 100644 --- a/src/editor.c +++ b/src/editor.c @@ -1878,6 +1878,17 @@ static gchar *find_calltip(const gchar *word, GeanyFiletype *ft) g_free(scope); if (constructor_tags->len != 0) { /* found constructor tag, so use it instead of the class tag */ + TMTag *constructor_tag; + foreach_ptr_array(constructor_tag, i, constructor_tags) + { + if (constructor_tag->type == tm_tag_method_t && constructor_tag->scope != NULL) + { + g_free(constructor_tag->name); + constructor_tag->name = g_strdup(constructor_tag->scope); + g_free(constructor_tag->scope); + constructor_tag->scope = NULL; + } + } g_ptr_array_free(tags, TRUE); tags = constructor_tags; } ```
To reproduce use the following simple test file and trigger the calltip on `__init__`, then on `LocalClass` and then on `EncodedWord`. ```python class LocalClass(object): def __init__(self, arg1, arg2=None): super().__init__()
# custom class from above LocalClass() # class imported via global tags file EncodedWord()
# imported via global tags file in ctags format # EncodedWord /unknown 1;" kind:class signature:(*args, **kw) ```
However, this crashes Geany upon the last triggered calltip on `EncodedWord`. It seems in `tm_tags_find` on calculating the `tagCount` there is no `last` tag found and it is NULL, so the calculation is "0 - first +1" which results in wrong numbers obviously. I changed the calculation to `*tagCount = (last) ? last - first + 1 : 1;` to return 1 as `tagCount` when we did not find a `last` tag but only the first one.
This made it much more stable but I still got a few random crashes which I cannot reliably reproduce and also sometimes after all three calltips have been shown, the calltip on `LocalClass` is missing :(. Sorry for the vague description.
I'm not sure whether it is worth to proceed with this, it already required quite some efforts for a rather small and specific feature.
If anyone has a quick clue what I'm doing wrong, I could give it another try. Otherwise I think I'll leave it as is.
@eht16 What you do isn't the right approach. The problem is that you modify tag names of `constructor_tags` array which is obtained using `tm_workspace_find` so it points directly to the sorted tag manager array of tags. When you modify the tag name, the array isn't sorted any more which leads to unpredictable results.
But I think we don't have to modify the tags themselves, in this case we just need to modify the string we pass to the calltip. Something similar to this should work:
```diff diff --git a/src/editor.c b/src/editor.c index 7306418a8..c1a866616 100644 --- a/src/editor.c +++ b/src/editor.c @@ -1919,7 +1919,11 @@ static gchar *find_calltip(const gchar *word, GeanyFiletype *ft)
if (str == NULL) { - gchar *f = tm_parser_format_function(tag->lang, tag->name, + const gchar *tag_name = tag->name; + + if (constructor_method && g_strcmp0(constructor_method, tag->name) == 0) + tag_name = word; + gchar *f = tm_parser_format_function(tag->lang, tag_name, tag->arglist, tag->var_type, tag->scope); str = g_string_new(NULL); if (calltip.tag_index > 0) ```
@eht16 pushed 1 commit.
87ca905615cd99abd05da59656e287bc34e604a6 Use constructor method calltip only if available
@eht16 pushed 1 commit.
9ae174288c7f9adcf12e212ab234ad7ffeba69c8 Use constructor method calltip only if available
Thanks @techee! This was great. I also felt that modifying the tags array is not a good idea but I didn't see why. Thanks.
Your suggestion is great, I fine tuned it a bit to not work on `__init__` tags directly (then their scope would be removed) because I needed to remove the scope from the calltip text if the tag name was replaced (otherwise we would have got "LocalClass.LocalClass (...)".
Tested also with some D code. I moved the two special cases out of "find_calltip" to make it a bit less cluttered. For me, this is now fine.
@techee commented on this pull request.
Looks good to me apart from the minor comments above.
@@ -1843,28 +1843,9 @@ static gint find_start_bracket(ScintillaObject *sci, gint pos)
}
-static gchar *find_calltip(const gchar *word, GeanyFiletype *ft) +static GPtrArray *update_tags_for_constructor_calltip(GPtrArray *tags, GeanyFiletype *ft, + TMTag *tag, const gchar *constructor_method)
I think it's a bit confusing for the reader of the code to see what this function does - it does a bit too many things. I think it would be better to call it something like `get_constructor_tags()` without passing the `tags` argument to it so this function would return `GPtrArray *` when constructors are found or NULL otherwise. Then, later, when this function is called, the code would look like ```C GPtrArray *constructor_tags = get_constructor_tags(ft, tag, constructor_method); if (constructor_tags) { g_ptr_array_free(tags, TRUE); tags = constructor_tags; } ```
@@ -1886,6 +1867,53 @@ static gchar *find_calltip(const gchar *word, GeanyFiletype *ft)
g_ptr_array_free(constructor_tags, TRUE); } } + return tags; +} + + +static void update_tag_for_construtor_calltip(const gchar *word, TMTag *tag, const gchar **tag_name,
I would call this `update_tag_name_and_scope_for_calltip()` or something similar because `update_tag` feels like it changes the `TMTag`.
@@ -1886,6 +1867,53 @@ static gchar *find_calltip(const gchar *word, GeanyFiletype *ft)
g_ptr_array_free(constructor_tags, TRUE); } } + return tags; +} + + +static void update_tag_for_construtor_calltip(const gchar *word, TMTag *tag, const gchar **tag_name, + const gchar **scope, const gchar *constructor_method)
I'd suggest moving `constructor_method` in front of `tag_name` and `scope` so the input arguments are in front of the output arguments.
@eht16 pushed 1 commit.
1efa45da50dbf5ce2161ff54d8cdcb83d6794fdf Improve naming and function signatures
@eht16 commented on this pull request.
@@ -1886,6 +1867,53 @@ static gchar *find_calltip(const gchar *word, GeanyFiletype *ft)
g_ptr_array_free(constructor_tags, TRUE); } } + return tags; +} + + +static void update_tag_for_construtor_calltip(const gchar *word, TMTag *tag, const gchar **tag_name, + const gchar **scope, const gchar *constructor_method)
Done, thanks.
@eht16 commented on this pull request.
@@ -1886,6 +1867,53 @@ static gchar *find_calltip(const gchar *word, GeanyFiletype *ft)
g_ptr_array_free(constructor_tags, TRUE); } } + return tags; +} + + +static void update_tag_for_construtor_calltip(const gchar *word, TMTag *tag, const gchar **tag_name,
Done, thanks.
@eht16 commented on this pull request.
@@ -1843,28 +1843,9 @@ static gint find_start_bracket(ScintillaObject *sci, gint pos)
}
-static gchar *find_calltip(const gchar *word, GeanyFiletype *ft) +static GPtrArray *update_tags_for_constructor_calltip(GPtrArray *tags, GeanyFiletype *ft, + TMTag *tag, const gchar *constructor_method)
Good point, done.
Looks good to me apart from the minor comments above.
Thanks for the suggestions!
LGTM.
If nobody will stop me, I'm going to squash and merge this in a week.
Merged #3334 into master.
❤️🙏
❤️🙏
github-comments@lists.geany.org