-
Notifications
You must be signed in to change notification settings - Fork 223
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
improve color support for themes #590
Conversation
CI appears to be broken due to #589 (seems that the resource_retriever package hasn't been re-released yet with those changes) |
Yeah, we need to release a new resource_retriever and a new rviz2. |
@ros-pull-request-builder retest this please |
@mikeferguson FYI, the current failures look to be style failures with this patch. |
@clalancette style fixes are fixed - but there's some weird failure I can't seem to figure out:
|
Yeah, sorry, we are having some infrastructure issues at the moment. Those should be fixed once ros-infrastructure/ros_buildfarm#828 is merged. If that is the only failure, then you are good to go. |
@clalancette -- thanks for the quick feedback -- yeah that appears to be the only issue (I had forgotten to lint before opening the PR - thos issues are fixed). |
@ros-pull-request-builder retest this please |
@clalancette tests are now passing! |
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.
#else | ||
return QColor(Qt::gray); | ||
#endif | ||
if (role == Qt::TextColorRole && parent_ && parent_->getDisableChildren()) { |
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 Windows build is complaining that Qt::TextColorRole
is deprecated.
Docs suggest changing this to Qt::ForegroundRole
.
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.
Looks right to me on the Mac - want to rerun the other tests?
@mikeferguson thanks! |
based on ros-visualization/rviz#1319