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

Context-dependent suggestions #29

Closed
wants to merge 3 commits into from

Conversation

LinqLover
Copy link
Contributor

@LinqLover LinqLover commented Jul 21, 2019

See commit message for details. This feature completes suggestions in ContextVariableInspector (we also can use it in SqueakSheet).

Accessing the Context in ECContext also provides new opportunities for guessing the type in an interactive context, like suggested in #17.

LinqLover and others added 3 commits July 21, 2019 13:44
Merge with MrModder:master
- Models can provide a #completionContext
- ECController detects this and sends it to ECContext
- in ECContext, search for temporaries is extended to temporaries from context
@LinqLover
Copy link
Contributor Author

@LeonMatthes Any news on this? :-)

@LeonMatthes
Copy link
Owner

I tried to analyze your changes, but I've not been able to figure out what exactly they achieve for the end user.

Please provide more information on how to reproduce the introduced behavior and what problems exactly it solves.

It seems your first commit adds entries to the Autocompletion for the local variables in the Context. However, at least in the Debugger, these are already provided from parsing the code.
So what functionality is added here?
I also was unable to find out in what specific case the guessTypeFromContext method does anything?

The additions of guessTypeName and selectedClassOrMetaclass to the Inspector are welcome changes though, and could be merged, were they in their own PR.

Please understand that I'm no longer an active Squeak user and reviewing complicated pull requests without detailed descriptions takes a lot of effort and keeps me from working on them.

@LinqLover
Copy link
Contributor Author

Sorry for the very late reply, @LeonMatthes, I have myself been much too busy during the latest months ...

Sorry for not explaining this PR properly, it is already pretty old. But still works in the latest Trunk image. :-)

The changes are visible when you type into the context variable inspector of a debugger:

image

Without this patch, the completion menu is missing any temporary variables from the current stack frame:

image

It seems your first commit adds entries to the Autocompletion for the local variables in the Context. However, at least in the Debugger, these are already provided from parsing the code.

This only applies to the main code pane of the debugger, but not to the inspectors at the bottom.

@LeonMatthes
Copy link
Owner

Alright, I was now able to integrate your changes into the Autocompletion.

Instead of passing the context to the ECContext/ECController to handle, I expanded and used the completionAdditionals API previously only available to the Workspace class.

In my opinion, this is a much cleaner approach that also opens up a much easier way to integrate future info provided by the model and requires only 2 new methods.

I've added you as a co-author to the commits that feature your code, because of how difficult it is to merge, I'll have to close this PR. But all your changes should be integrated.

@LeonMatthes LeonMatthes closed this Nov 1, 2021
@LinqLover
Copy link
Contributor Author

Sounds fine, thank you for integrating these changes! :-)

@LinqLover LinqLover deleted the patch-3 branch November 14, 2021 17:28
@LinqLover
Copy link
Contributor Author

LinqLover commented Nov 14, 2021

(FYIO, today I found two other implementors of #completionContext in my images which I now rewrote by coping over the implementation from ContextVariablesInspector >> #guessTypeForName:. It still works, but if you should decide to rework this protocol in the feature, I would still be glad to have this hook. :-))

@LinqLover LinqLover restored the patch-3 branch November 14, 2021 17:34
@LeonMatthes
Copy link
Owner

(FYIO, today I found two other implementors of #completionContext in my images which I now rewrote by coping over the implementation from ContextVariablesInspector >> #guessTypeForName:. It still works, but if you should decide to rework this protocol in the feature, I would still be glad to have this hook. :-))

Okay, thanks for the info.
It generally depends on where you want to put the burden of dealing with the context.
Currently, Autocompletion doesn't have to deal with it, as the Models have to map these from a Context into completionAdditionals and do the type guessing.
If we were to add the API requested in this PR, this burden would be moved into the Autocompletion, whilst providing a more convenient API.
As I naturally want to keep the API hat hooks into the Autocompletion as general and easy to deal with as possible (from Autocompletions side 😅 ) I don't think I'll ever add this.
However, to make dealing with Contexts easier, maybe the Code could be moved into the Context instead.

Then one would just have to write:

completionAdditionals
    ^ self doItContext ifNotNil: #completionAdditionals

Or however access to the models context works...

@LinqLover
Copy link
Contributor Author

+1 for Context >> #completionAdditionals, this might save me from some duplication in the long term. :-)

Nevertheless, here is another argument for moving this burden into Autocompletion: its analogy to the Shout (Syntax Highlighting As yoU Type) styling interface. Already back in 2019 when working on SqueakSheet, I observed that most models needed to implement both interfaces to provide nearly the same information, i.e. all data required for parsing the code in context (class, environment, context, whether there is code at all, etc.). If I were to write a new completion tool (but currently, I am not), I would probably even reuse the information provided by #aboutToStyle:requestor: to avoid this duplication. And there is also SHTextStyler >> #context:. By the way, I am also working on a Sandbox-powered extension to Autocompletion that also needs the #completionContext because a list of variable names is not sufficient for this extension.

Anyhow, I understand and respect your point of API simplicity from above, so please take the above paragraph as some rather general and perspective thoughts only. :-)

@LinqLover LinqLover deleted the patch-3 branch August 29, 2023 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants