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

#608, #229 Reveal the error location after the failed verify #1064

Merged
merged 2 commits into from
Jun 21, 2022

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jun 15, 2022

Motivation

compiler_errors_01.mp4

Screen Shot 2022-06-15 at 16 15 54

  • Show the first compilation error in the notification center.

Screen Shot 2022-06-15 at 16 17 01

  • Can copy the Output content from the error notification:
compiler_errors_02.mp4

Experimental

You need to enable the experimental features from the settings.json if you want to see "more errors" 😉:

  • Open the Command Palette with F1,
  • Type and Preferences: Open Settings (JSON) and press Enter,
  • Add "arduino.compile.experimental": true to the JSON, save the file if you do not have auto-save enabled, and close the settings.json editor.
  • Compile a sketch with multiple errors. Here is an example sketch:
    Errors.zip

It should be possible to navigate between the errors.

compiler_errors_03.mp4

Expected behavior:

  • The order of the compiler errors is given by the CLI (The order of the compiler errors are indeterministic arduino-cli#1760),
  • The Next Error and the Previous Error navigation depend on the order of the compiler errors,
  • The error parsing logic in IDE2 is expected to be identical as in the Java IDE.
  • If the experimental features are enabled, but there is one compiler error, navigation is not available.
  • If the cursor location intersects the location of the compiler error, or in the same line, the error under the cursor will be the current error and will have the previous/next action. (There is an issue when there are multiple errors in the same line or location. I am working on this. 🚧 #608, #229 Reveal the error location after the failed verify #1064 (comment) Fixed: 4f486cb)
  • The error decoration (red line) must be sticky. It means, that editing the file should move the markers around.
  • If a code is deleted from an error decoration location, or text is inserted into an error decoration location, the error decoration will be removed.
compiler_errors_04.mp4

Known issues:

  • When deleting code from a location with the error decoration.
compiler_errors_05.mp4
  • TBD,

Open questions:

  • As noted in the code, I do not know which focusing logic prefers the community the best. So I would like to hear feedback. I will add a preference for this soon. I added a preference: 54e60f6

Change description

Other information

Closes #608
Closes #229

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos changed the title [WIP]: Reveal the error location after on failed verify. [WIP]: Reveal the error location after the failed verify Jun 15, 2022
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jun 15, 2022
export const UploadUsingProgrammerFailed = create(
Codes.UploadUsingProgrammer
);
export const BurnBootloaderFailed = create(Codes.BurnBootloader);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice to have: burn bootloader is unrelated to the sketch content, IDE2 won't ever get a location, so consider having a different error type.

): ApplicationError.Constructor<number, ErrorInfo[]> {
return ApplicationError.declare(
code,
(message: string, data: ErrorInfo[]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice to have: consider replacing message with a ServiceError instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ServiceError object does not contain sufficient info about the error, so this won't fix.

Ref: arduino/arduino-cli#1767

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

It may be out of scope for this PR, but I think the color used for stderr in the output panel with the "Light (Arduino)" theme must be made lighter.

Here is how it looks in Arduino IDE 1.8.19:

1 8 19-stderr

Compare that to the color when using the build from this PR:

2 x-stderr

At one point, this color in Arduino IDE 1.x was changed to a similar darker red until it was restored to the current color after complaints about readability from the users:

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: resolved

More of a "nice to have" than something really essential, but I thought I would mention it since I did notice it by chance.

Would it be possible to still get selection highlighting on the text of the line with the error highlighting?

Notice how the selected text on the error line has a yellow background in Arduino IDE 1.8.19:

image

While there is no visual indication of the same selection on that line when using the build from this PR:

image

@kittaakos
Copy link
Contributor Author

Thank you for trying it out!

It may be out of scope for this PR, but I think the color used for stderr in the output panel with the "Light (Arduino)" theme must be made lighter.

I propose opening a follow-up for this. Currently, IDE2 uses the default --theia-inputValidation-errorBackground value. The designer team can work on it and align the default Theia "error color" to the Arduino theme.

Would it be possible to still get selection highlighting on the text of the line with the error highlighting.

Nice catch! Sure. I look into it.

@kittaakos
Copy link
Contributor Author

Notice how the selected text on the error line has a yellow background in Arduino IDE 1.8.19:

image

While there is no visual indication of the same selection on that line when using the build from this PR:

image

✅ Fixed

selection_fix.mp4

@kittaakos kittaakos force-pushed the #608-signed branch 2 times, most recently from 14b71e4 to 3a17550 Compare June 17, 2022 09:18
@kittaakos kittaakos changed the title [WIP]: Reveal the error location after the failed verify #608, #229 Reveal the error location after the failed verify Jun 17, 2022
@kittaakos kittaakos marked this pull request as ready for review June 17, 2022 09:21
@kittaakos
Copy link
Contributor Author

I fixed all outstanding issues and made the PR ready for review. Please verify. Thank you!

@per1234 per1234 linked an issue Jun 19, 2022 that may be closed by this pull request
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

It is working great for me.

Thanks for this very valuable advancement Akos!

Akos Kitta added 2 commits June 21, 2022 10:04
Closes #608
Closes #229

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos
Copy link
Contributor Author

kittaakos commented Jun 21, 2022

@davegarthsimpson please review e7ee861 for #1074. I verified it locally, but I would appreciate a double-check. Thank you!

Update: here is the link to the module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
4 participants