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

refactor: Adjust inlay rendering and improve completion logic #3494

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

lkk214
Copy link
Contributor

@lkk214 lkk214 commented Dec 21, 2024

Description

  • Single-line completion sometimes does not work
  • There is a blank area after the cursor when completing multiple lines
  • Incompatible line breaks cause accept to fail

Screenshots

fix14

Testing

fix15

- Replace `ContinueCustomElementRenderer` and `ContinueMultilineCustomElementRenderer` with `ContinueInlayRenderer`.
- Add `Editor.addInlayElement` to handle inline and block inlay elements.
- Refactor `AutocompleteService` to use new inlay renderer and improve completion rendering conditions.
- Update `AcceptAutocompleteAction` to match new changes.
Copy link

netlify bot commented Dec 21, 2024

Deploy Preview for continuedev canceled.

Name Link
🔨 Latest commit 79736c3
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/67741b3a79f1ff00089f9d2d

Copy link
Contributor

@sestinj sestinj left a comment

Choose a reason for hiding this comment

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

This works great for me, I'm just curious if the url -> path was the solution to something that was previously broken

val column = editor.caretModel.primaryCaret.logicalPosition.column
val input = mapOf(
"completionId" to completionId,
"filepath" to virtualFile?.url,
"filepath" to virtualFile?.path,
Copy link
Contributor

Choose a reason for hiding this comment

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

why change url to path?

Copy link
Contributor

Choose a reason for hiding this comment

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

(was this the solution to a clear problem?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just about to comment on it. I didn't pay attention to #3247, need to revert it here.

Because using File(URI(filepath)) will cause an error in Windows, I made temporary changes locally and submitted this by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, maybe it's better to fix it than revert it.

@lkk214
Copy link
Contributor Author

lkk214 commented Dec 31, 2024

Also, add directory checks and optimize token usage.

  • Added check to skip directories when reading file content.
  • Optimized token usage by removing unnecessary \r characters.

- Added check to skip directories when reading file content.
- Optimized token usage by removing unnecessary `\r` characters.
@sestinj
Copy link
Contributor

sestinj commented Jan 8, 2025

Looks good! @Patrick-Erichsen for a second pass, then good to merge

}

fun VirtualFile.toUriOrNull(): String? = fileSystem.getNioPath(this)?.toUri()?.toString()?.removeSuffix("/")
Copy link
Collaborator

@Patrick-Erichsen Patrick-Erichsen Jan 8, 2025

Choose a reason for hiding this comment

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

@RomneyDa any thoughts here? I'm wondering if .removeSuffix("/") is going to not work properly for non-macOS users

@Patrick-Erichsen
Copy link
Collaborator

LGTM 🫡

@sestinj sestinj merged commit 4b97663 into continuedev:main Jan 10, 2025
4 checks passed
@AfterStories
Copy link

AfterStories commented Jan 11, 2025

@lkk214 @Patrick-Erichsen @sestinj
I still found new similar problems, I think it is also caused by the same bug, but it has not been fixed:
When I select a piece of code and use the CTRL+I shortcut key, as shown in the screenshot, you can see that the model name is not displayed, only show a null

And at the same time, the log in the Run Extension window:

Error handling message: {"messageType":"config/getSerializedProfileInfo","data":{"result":{"config":{"allowAnonymousTelemetry":false,"models":[{"provider":"anthropic","model":"Gosu Model","title":"Gosu Model"... ....(After here, it's my config. json content) 
java.lang.NullPointerException: null cannot be cast to non-null type kotlin.collections.Map<kotlin.String, kotlin.Any>
Registered handler for editor

image
image

@lkk214
Copy link
Contributor Author

lkk214 commented Jan 11, 2025

From the log you provided, it seems that it is a response parsing error. It is not the same issue.
I have not updated the code, so it works normally locally. The relevant code for response parsing:

continuePluginService.coreMessenger?.request("config/getSerializedProfileInfo", null, null) { response ->
val config = response as Map<String, Any>
val models = (config["config"] as Map<String, Any>)["models"] as List<Map<String, Any>>
modelTitles.addAll(models.map { it["title"] as String })
}

@AfterStories
Copy link

AfterStories commented Jan 14, 2025

From the log you provided, it seems that it is a response parsing error. It is not the same issue. I have not updated the code, so it works normally locally. The relevant code for response parsing:

continuePluginService.coreMessenger?.request("config/getSerializedProfileInfo", null, null) { response ->
val config = response as Map<String, Any>
val models = (config["config"] as Map<String, Any>)["models"] as List<Map<String, Any>>
modelTitles.addAll(models.map { it["title"] as String })
}

Yes, I also found this code, and I saw that it made a "config/getSerializedProfileInfo" request to get the content of the config,
but maybe there was a problem in the kt code post message to the “core” code ,so failed to get the correct response.
Or the core code failed to parse the config.json file and return its content,I am not understand the message manager part of the kt code to core layer code

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.

4 participants