Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for scoped function calltips #1177

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

krogank9
Copy link

@krogank9 krogank9 commented Aug 12, 2016

I created function read_scope_prefix which is similar to read_current_word. It gets the scope of the word the cursor is on:

if the user types: ScopeA::ScopeB::Function
it will return ScopeA::ScopeB

Then I used my read_scope_prefix function in editor_show_calltip where find_calltips is called. To find_calltips I added an argument const gchar *scope to send the value over. find_calltips now passes scope argument to tm_workspace_find function. If no tags are found with matching scope, find_calltips falls back to its original behavior, which does not account for scope and returns all calltips with the given tag name, in any scope.


As shown in the image, the top one is how it was before my patch. It will not correctly display calltips for namespaces/static class functions. When there are multiple classes which have functions of the same name, they conflict. My patch restricts the functions displayed to the current scope, if available.

//symbols_get_current_scope(editor->document, &sname);
//printf("Current scope: %s\n", sname);
read_scope_prefix(editor, pos - 1, scope, sizeof scope, NULL);
str = find_calltip(word, scope, editor->document->file_type);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have liked to use symbols_get_current_scope here rather than write my own read_scope_prefix function, but symbols_get_current_scope kept returning "unknown" for some reason. if anyone knows how to make that function work please do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symbols_get_current_scope() returns a somewhat limited view of the lexical scope IIUC, so its not what you want until you need to resolve the highest level name in your prefix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I realized that. I removed those comments. Now I know I can't use it

@b4n
Copy link
Member

b4n commented Aug 20, 2016

This should be streamlined as much as possible with autocomplete_scope(), which does mostly the same as what you're trying to achieve, and can probably share some code.

@b4n
Copy link
Member

b4n commented Aug 20, 2016

@techee might be able to help here :)

@techee
Copy link
Member

techee commented Aug 20, 2016

Yeah, looks similar and probably both cases could share some code. This patch works for namespace prefixes only but it could also be changed to work for variable prefixes too (for strongly-typed languages for which we have varType).

On the other hand this patch supports multiple levels of scopes which we don't have in scope completion right now (the code would probably get quite a bit crazier if we were doing scope completion for multiple levels). But maybe it's not so important and one level could be enough.

But yeah, the idea behind the patch makes sense and brings back the beautiful memories of the implementation of scope completion :-).

@krogank9
Copy link
Author

krogank9 commented Aug 22, 2016

Looking at autocomplete_scope I don't see any way this could share its code as the patch stands now. But I find the way that tm_workspace_find returns tags to find_calltips to be satisfactory. For example all of those MeshBuilder and VertexData namespaces are technically within the namespace BABYLON (which wasn't recorded in my ctags file). I can type 'BABYLON.MeshBuilder.CreateBox' which will have the scope prefix BABYLON.MeshBuilder. tm_workspace_find will return all tags with scopes matching the last part of the scope. So I can also type 'Whatever.Scope.MeshBuilder.CreateBox(' and it will show scoped calltips correctly for the MeshBuilder namespace. And you can still type just 'CreateBox(' or 'nonexistantnamespace.CreateBox(' to get all the functions named CreateBox, in any namespace.

Yes it would be very cool to also extend this to work for variable prefixes too in strongly typed languages. I have a bit of reading to do to see what you guys have implemented on that end so far. I know there's some support for CPP-specific autocompletion already. Class instances in C++ will show their correct function autocompletion even when there are clashing function names across multiple classes which is pretty awesome, but once the calltip is shown it no longer respects the scope so well and it just shows all functions. If we implemented the equivalent for calltips, it definitely looks like it could share some code. Is that the autocomplete_scope function doing that magic? It looks like it uses symbols_get_current_scope which I couldn't get to work here

@elextr
Copy link
Member

elextr commented Aug 22, 2016

but once the calltip is shown it no longer respects the scope so well and it just shows all functions.

Yeah, calltips don't look at context AFAIK, only scope autocomplete does, for named scopes (ie classes, namespaces) only, it doesn't know about lexical scopes. And variable scope auto-completion only works for variables defined at top level, so its isn't very useful for modern C/C++.

The whole area could probably use a re-think instead of piling patches over patches, especially if @techee doubts it.

@techee
Copy link
Member

techee commented Aug 22, 2016

@krogank9 I'll try to do the necessary changes on the TM side so you can use it for this feature.

@b4n
Copy link
Member

b4n commented Aug 22, 2016

What's the problem with TM? I might be missing something obvious, but a quick and dirty patch like http://pastebin.geany.org/ISAQT/ seems to mostly work.

It could possibly be improved to use the current scope to resolve direct methods and some other stuff, but that could be added. I mean like

class Foo {
    std::string something(int n) {
        return nullptr;
    }

    int main() {
        return something(/* here, show Foo::something() only, not Bar::something() */);
    }
};

class Bar {
    int something() {
        return 0;
    }
};

/*
But this works:

Foo::something();
Bar::something();

class Z {
    Foo memb;
    Z() {
        memb.something();
    }
};
*/

@techee
Copy link
Member

techee commented Aug 22, 2016

Ah right, I forgot tm_workspace_find_scope_members() returns functions too which is what I thought would have to be added. Everything there then, no work for me :-P.

@b4n
Copy link
Member

b4n commented Aug 22, 2016

@techee yeah, you'll review, and possibly add support for implicitly scoping on this :) Although scoping on this might be tricky, because you still want to allow global variables and methods. Methods should be fine as the this one would have precedence anyway, and we know which it is, but scope completion would have to consider both the this scope and the global one in this case.

@b4n
Copy link
Member

b4n commented Aug 22, 2016

Ah, and should I PR my quick patch for further discussion/work/whatever, or it's too quick'n dirty do be of any actual use?

@techee
Copy link
Member

techee commented Aug 22, 2016

Ah, and should I PR my quick patch for further discussion/work/whatever, or it's too quick'n dirty do be of any actual use?

Would be good - I'll review and also @krogank9 can check if it works for his use cases.

@krogank9
Copy link
Author

@techee Yup I tried it out, it works the same as my patch and also adds support for variables/instances of a class for strongly typed languages, not just for use with namespaces/static methods

b4n added a commit to b4n/geany that referenced this pull request Aug 22, 2016
Try and select the correct calltip based on scope information on the
left of the symbol triggering the calltip, similar to scope completion.

Closes geany#1177.
@b4n b4n mentioned this pull request Aug 22, 2016
@elextr
Copy link
Member

elextr commented Aug 23, 2016

add support for implicitly scoping on this :)

And @techee can add support for C++ scoping of class members in member functions where this is implicit, so I have less to complain about "every language is C" :)

@b4n
Copy link
Member

b4n commented Aug 23, 2016

add support for C++ scoping of class members in member functions where this is implicit

Mmm, what? Isn't it what I did, that is that it considers the current scope when there's nothing on the left? My second commit doesn't try to handle this.foo() (which would be nice, but few languages use this), but foo() inside a scope.
The idea being that if it's inside Foo::bar, it has to be part of one of the scopes Foo::bar, Foo, or none; but e.g. not Baz, as it wouldn't be accessible without an explicit prefix (unless inherited somehow, but we don't have that yet anywhere).

@elextr
Copy link
Member

elextr commented Aug 23, 2016

@b4n, it looked like you are relying on autocomplete's algorithm by cunningly renaming it, but autocomplete doesn't work AFAICT, eg:

struct Bar {
    int aaab;
};

struct Foo {
    int aaaa;
    int f(){
        aa // now autocomplete shows both aaaa and aaab
    };
};

Or is that not what your patch doing.

[edit: clarify two uses of "it" :) ]

@b4n
Copy link
Member

b4n commented Aug 23, 2016

it looked like you are relying on autocomplete's algorithm by cunningly renaming it

what does this mean?

Anyway, your example works for me if there is only one Foo in the workspace. I'm not very sure why I might pick up the wrong Foo if there are more even though I pass editor->document->tm_file, but well.

@elextr
Copy link
Member

elextr commented Aug 23, 2016

it looked like you are relying on autocomplete's algorithm by cunningly renaming it
what does this mean?

I meant this change:

-static gboolean autocomplete_scope(GeanyEditor *editor, const gchar *root, gsize rootlen)
+static GPtrArray *get_scoped_tags(GeanyEditor *editor, gint pos)

Now my turn :), what do you mean by "only one Foo", the symbol aaab is in Bar not Foo, but it shows for autocomplete inside a member function of Foo

@b4n
Copy link
Member

b4n commented Aug 23, 2016

I mean rename Foo to FooImSureThereIsOnlyOne and it'll work.

[edit] the reason why you see aaab is that if nothing matched the normal non-scope completion kicks in and you then get everything.

@elextr
Copy link
Member

elextr commented Aug 23, 2016

Nope, not in current Geany, still both. Or is it some extra magic you added in the patch as well as making scope completion apply to calltips?

@b4n
Copy link
Member

b4n commented Aug 23, 2016

Oooooh, okay, sorry, I assumed you were trying #1188 which does add it.

@elextr
Copy link
Member

elextr commented Aug 23, 2016

Oh, havn't even looked at #1188 yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants