-
Notifications
You must be signed in to change notification settings - Fork 92
Span inspector #1296
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
base: main
Are you sure you want to change the base?
Span inspector #1296
Conversation
This highlights all output tokens that share the same span as the text cursor is hovering over.
Expansion erros are now shown directly in the output textfield.
- Fix incorrect calculation of target span - Fix false highlighting caused by Pretty Please code manipulation
- Prevents issues with special characters like '<' and '>' breaking the output HTML
These tokens are now ignored during parsing because prettyplease inserts them automatically in certain situations.
…fresh bug - Make output text area scrollable - Adjust text color to match the theme - Fix edge case where text change did not trigger refresh if cursor position stayed the same
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1296 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 75 75
Lines 12772 12784 +12
=========================================
+ Hits 12772 12784 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Before tests were failing due to an outdated Cargo.lock
- Tests were failing due to an outdated Cargo.lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app works nicely and the overall approach is fine.
There are still some improvements to the code necessary before we can merge though.
let string = token.to_string(); | ||
!matches!(string.as_str(), "," | "}" | "{") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is implemented twice in build_html, please factor it out into a common method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I think pretty-please can also insert ,
so that may need to be added to the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I think pretty-please can also insert
,
so that may need to be added to the list.
I am a bit confused, because ,
is already included into the filter. If you meant .
, I don't think it is possibly for pretty-please to insert them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I think pretty-please can also insert
,
so that may need to be added to the list.I am a bit confused, because
,
is already included into the filter. If you meant.
, I don't think it is possibly for pretty-please to insert them.
Sorry for the confusion, yes, it's alreay in the list. Don't know why I didn't notice 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way: Fancy way of adding that filter_pretty_please method with an extension trait.
Somewhat over-engineered, but we can leave it in.
background-color: #ff00ff; | ||
padding: 2px 6px; | ||
border-radius: 6px; | ||
} | ||
.generated { | ||
color: rgba(255, 255, 255, 100); | ||
padding: 2px 6px; | ||
border-radius: 6px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These colors are hard-coded, and can be hard to read, depending on the color scheme (light/dark mode).
Ideally these would be chosen based on the current palette of the widget, but this may be hard to fix at the moment, so it's something to note for a follow-up PR.
}; | ||
qt_thread | ||
.queue(move |this| { | ||
unsafe { this.output_document() }.set_html(&QString::from(output)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do multiple subsequent edits to the input string, they all race to set their result as the output_string. If a later edit finishes faster, it will be overwritten by an edit that happened beforehand, leaving the app in an inconsistent state.
The easiest way to fix this is to add a request_token
(just an i32) that is incremented every time a background thread is started.
Only the background thread with the matching request_token is then allowed to set its result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The THREAD_COUNT needs to be a member variable inside SpanInspector, not a global static.
Otherwise instantiating two span inspectors will misbehave.
It then also doesn't need to be atomic, but can just be a normal integer. Other than that it's correctly implemented. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuentinLeeWeber Once this is fixed, this PR is ready for approval.
- thread_count is no longer a static atomic type, so multiple span-inspectors can be used without interferrring their selfs
This PR introduces a tool called span-inspector that helps analyze and visualize the spans of tokens generated from the input to a cxx-qt-bridge macro.
Details