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.

Sign up for GitHub

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

Jump to bottom

#6278: discard newlines, collapse and trim whitespaces. #6280

Merged
merged 2 commits into from
Aug 2, 2023
Merged

#6278: discard newlines, collapse and trim whitespaces. #6280

merged 2 commits into from
Aug 2, 2023

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Aug 1, 2023

NBLS New from Template command implementation strips HTML from description of templates, but leaves whitespaces as they were in the html source, so potentially more (white)spaces than necessary, including tab/newline control characters. vscode now (from June's releaase 1.80.2) starts to display control characters in quickpick UI - which looks ugly, see #6278.

This PR:

  • removes all newlines
  • coalesces whitespaces into a single space
  • kills leading/trailing whitespaces

Should apply only to description of templates.

Please do consider to include this fix in NB(LS) 19 release, the New from Tempalate looks really ugly.

@sdedic sdedic added LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests priority:high High priority issue that should, if possible, be fixed in next release labels Aug 1, 2023
@sdedic sdedic added this to the NB19 milestone Aug 1, 2023
@sdedic sdedic requested review from dbalek and MartinBalin August 1, 2023 07:31
@sdedic sdedic self-assigned this Aug 1, 2023
@sdedic sdedic linked an issue Aug 1, 2023 that may be closed by this pull request
Ugly characters in VSNetBeans wizards #6278
Closed
@sdedic sdedic changed the base branch from master to delivery August 1, 2023 07:31
@neilcsmith-net
Copy link
Member

Makes sense - will leave @MartinBalin to test usage and merge - rc4 will be Thursday earliest.

Slightly surprised at treating \n separately to other whitespace. I assume there's a good reason for that?

@sdedic
Copy link
Member Author

sdedic commented Aug 1, 2023

@neilcsmith-net good catch; I will be appending/changing this PR as there are more places affected by the vscode change. I'll unify the handling in the stripping routine.

@sdedic sdedic force-pushed the lsp/quickpick-whitespaces branch from 0bd05e4 to afd61d1 Compare August 1, 2023 12:08
@sdedic
Copy link
Member Author

sdedic commented Aug 1, 2023

Restructured, added Changelog in a separate commit.
I have injected the whitespace handling into LanguageClient wrapper, so every showQuickPick message coalesces whitespaces and strips newlines.

@sdedic sdedic force-pushed the lsp/quickpick-whitespaces branch from afd61d1 to 76e8810 Compare August 1, 2023 12:11
dbalek
dbalek approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

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

Looks fine. Thanks !

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/LspTemplateUI.java Outdated
}
}
return sb.toString();
return Utils.html2plain(s, true).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to call trim() again.

neilcsmith-net
neilcsmith-net reviewed Aug 1, 2023
View reviewed changes
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/Utils.java Outdated
ch = ' '; // NOI18N
whitespace = true;
} else {
if (insertWSIfMissing) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this ever true?

Copy link
Member Author

Choose a reason for hiding this comment

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

result of merging two implementations together ;)

@sdedic sdedic force-pushed the lsp/quickpick-whitespaces branch from 9798aa5 to 748ef78 Compare August 2, 2023 08:11
@sdedic
Copy link
Member Author

sdedic commented Aug 2, 2023

Squashed to just have just changelog update separate.

@MartinBalin MartinBalin merged commit 38ba76c into apache:delivery Aug 2, 2023
@MartinBalin
Copy link
Contributor

@neilcsmith-net was this merged to master and release 19 branches? The bug can be still seen in build from master... thanks

@neilcsmith-net
Copy link
Member

@MartinBalin sync to master #6276 was only done a few hours ago so likely your master is not up to date? Ran out of time to check and sync after doing 19-rc4 on Friday.

@MartinBalin
Copy link
Contributor

No problem and thank you.

@neilcsmith-net
Copy link
Member

Note that there's always a delay (just usually not that long) between merging to the release branch and to master. We do the release candidate build in between the two sync PRs to check everything is OK before merging into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilcsmith-net neilcsmith-net neilcsmith-net left review comments

@dbalek dbalek dbalek approved these changes

@MartinBalin MartinBalin MartinBalin approved these changes

Assignees

@sdedic sdedic

Labels
LSP [ci] enable Language Server Protocol tests priority:high High priority issue that should, if possible, be fixed in next release VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Milestone
NB19
Development

Successfully merging this pull request may close these issues.

Ugly characters in VSNetBeans wizards
4 participants
@sdedic @neilcsmith-net @MartinBalin @dbalek