Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

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

Merged
kittaakos merged 2 commits into main from #608-signed
Jun 21, 2022

Conversation

@kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jun 15, 2022 •
edited
Loading

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

@kittaakos kittaakos Jun 15, 2022

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

@kittaakos kittaakos Jun 15, 2022

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

@kittaakos kittaakos Jun 17, 2022

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 •
edited
Loading

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 reacted with thumbs up emoji
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.

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
Copy link
Contributor Author

I fixed all outstanding issues and made the PR ready for review. Please verify. Thank you!
per1234 reacted with eyes emoji

@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>
Copy link
Contributor Author

kittaakos commented Jun 21, 2022 •
edited
Loading

@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.

@kittaakos kittaakos merged commit 4611381 into main Jun 21, 2022
@kittaakos kittaakos deleted the #608-signed branch June 21, 2022 11:09
@per1234 per1234 added the conclusion: resolved Issue was resolved label Jun 21, 2022

Reviewers

@davegarthsimpson davegarthsimpson davegarthsimpson left review comments

@per1234 per1234 per1234 approved these changes

@fstasi fstasi Awaiting requested review from fstasi

@ubidefeo ubidefeo Awaiting requested review from ubidefeo

+1 more reviewer

@AlbyIanna AlbyIanna AlbyIanna left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

AltStyle によって変換されたページ (->オリジナル) /