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

fix multiline highlighting was ~add ci test for non optional build~ #594

Merged
merged 2 commits into from
Apr 15, 2024
Merged

fix multiline highlighting was ~add ci test for non optional build~ #594

merged 2 commits into from
Apr 15, 2024

Conversation

lievenhey
Copy link
Contributor

This commit adds a ci build to build hotspot without optional dependencies. This should prevents bugs like #575 (fails to build without qcustomplot).

@lievenhey
Copy link
Contributor Author

I encountered a similar problem when I tried debugging my changes. In that case it was impossible to build hotspot without KSyntaxHighlighter. The ci update should catch that in the future.

@lievenhey
Copy link
Contributor Author

Nice, the ci found the first bug already

@lievenhey lievenhey linked an issue Jan 16, 2024 that may be closed by this pull request
Source Code view only C* "highlights" multiline comments on the first line #584
Closed
@lievenhey lievenhey force-pushed the fix-highlighting branch 5 times, most recently from c14b27f to a3b2909 Compare January 16, 2024 14:02
@lievenhey
Copy link
Contributor Author

Multi line comments are now highlighted correctly
grafik

@lievenhey lievenhey requested a review from milianw January 16, 2024 14:04
@lievenhey
Copy link
Contributor Author

dammit, it broke the disassembler

@lievenhey
Copy link
Contributor Author

now it is working correctly

@GitMensch
Copy link
Contributor

Nice work!

the PR's title may should be adjusted, or the changes split into two PRs (I don't think that's necessary as long as it is split in the commits)

milianw
milianw requested changes Jan 17, 2024
View reviewed changes
src/models/highlightedtext.cpp Outdated Show resolved Hide resolved
src/models/highlightedtext.cpp Outdated Show resolved Hide resolved
tests/modeltests/tst_formatting.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey changed the title add ci test for non optional build fix multiline highlighting was ~add ci test for non optional build~ Jan 22, 2024
@lievenhey lievenhey force-pushed the fix-highlighting branch 3 times, most recently from f13a9e2 to f5aef07 Compare January 22, 2024 10:20
@lievenhey
Copy link
Contributor Author

I split the ci change off into its own pr

@GitMensch
Copy link
Contributor

I do wonder if not resetting the highlighter has any performance impact

@lievenhey
Copy link
Contributor Author

It has. Until this patch the Highlighter was called on demand. Now it is called once for the whole file.

@lievenhey lievenhey force-pushed the fix-highlighting branch 2 times, most recently from c76fd1c to 411d7b9 Compare January 22, 2024 14:08
@GitMensch
Copy link
Contributor

I guess that's good to go after solving the merge conflict.

@GitMensch
Copy link
Contributor

Just wondering: Is there anything blocking the pull here?

@lievenhey lievenhey added the bug label Feb 6, 2024
@lievenhey
Copy link
Contributor Author

Milians high workload.

milianw
milianw approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

can you also add a test for this please? otherwise feel free to merge as-is for now already

@lievenhey
don't reset KSyntaxhigHlighter state every line
404e01e 
If the state is reset every line KSyntaxHighlighter will be unable to
highlight multi line comments (for example). This patch changes that by
highlighting everyting at once.
@lievenhey
add test for multiline highlighting
9f76aac
@lievenhey lievenhey merged commit e8f04fc into master Apr 15, 2024
25 checks passed
@lievenhey lievenhey deleted the fix-highlighting branch April 15, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milianw milianw milianw approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source Code view only C* "highlights" multiline comments on the first line
3 participants
@lievenhey @GitMensch @milianw