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.

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

feat: add an env variable to enable the new rviz2 theme #1017

Merged
merged 14 commits into from
Sep 12, 2024

Conversation

KhalilSelyan
Copy link
Contributor

@KhalilSelyan KhalilSelyan commented Jun 4, 2024

Description

Please see:

This just adds the required env var to the launch file to enable the new theme.

Related links

Tests performed

I've confirmed by uninstalling qt5ct I was able to run rviz with older theme with this PR enabled.

And with this enabled, and

Notes for reviewers

  • This PR should be reviewed alongside any related PRs that involve updates to the UI or styling of RViz.
  • Ensure that the setup script is thoroughly tested in different environments to confirm its robustness.

Interface changes

None.

Effects on system behavior

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@KhalilSelyan KhalilSelyan added the component:ui User interface, user experience, graphical user interfaces. (auto-assigned) label Jun 4, 2024
@KhalilSelyan KhalilSelyan self-assigned this Jun 4, 2024
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) and removed component:ui User interface, user experience, graphical user interfaces. (auto-assigned) labels Jun 4, 2024
@KhalilSelyan KhalilSelyan marked this pull request as ready for review June 5, 2024 18:00
@KhalilSelyan KhalilSelyan added component:ui User interface, user experience, graphical user interfaces. (auto-assigned) and removed type:documentation Creating or refining documentation. (auto-assigned) labels Jun 5, 2024
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) and removed component:ui User interface, user experience, graphical user interfaces. (auto-assigned) labels Jun 6, 2024
@xmfcx xmfcx marked this pull request as draft June 6, 2024 15:48
@xmfcx
Copy link
Contributor

xmfcx commented Jun 6, 2024

Please put the installation related things to the https://github.com/autowarefoundation/autoware/tree/main/ansible/roles

We don't want to run custom scripts in the CMakeLists files.

@github-actions github-actions bot removed the type:documentation Creating or refining documentation. (auto-assigned) label Jun 7, 2024
@KhalilSelyan
Copy link
Contributor Author

Please put the installation related things to the https://github.com/autowarefoundation/autoware/tree/main/ansible/roles

We don't want to run custom scripts in the CMakeLists files.
@xmfcx I have opened a separate PR on autoware repo with the ansible scripts update
autowarefoundation/autoware#4838

Copy link

stale bot commented Aug 6, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale label Aug 6, 2024
@xmfcx xmfcx force-pushed the qt5ct-dark-mode-integration branch from fb5c3bc to fd85658 Compare September 5, 2024 11:05
@xmfcx
Copy link
Contributor

xmfcx commented Sep 5, 2024

image

It looks like this for me, is this expected?

The button in the circled area looks weird for me.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 5, 2024

image

Could you make these less subtle (smaller shadow) and (bottom-centered) (as opposed to bottom-right)?

reference:
2024-09-05_15-15

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

could you make these selected line color same as the selected menu items?

image

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

Normal RViz
image

New theme
image

sorry for the screenshot quality, OS didnt let me screenshot while textbox was selected.

new theme adds an underline to the selected text in the textbox, could you remove that?

also i think there is a border around it now. but i think it will look good once it becomes gray with the comment above.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

image

this has issues

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

image

disabled things don't look disabled

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

these textbox/combobox dropdowns should also adopt the usual menu color scheme. and remove all rounded corners from them.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 11, 2024

image

image

these shouldn't have checkboxes and follow the menu color scheme. and corners are black for some reason.

@KhalilSelyan
Copy link
Contributor Author

@xmfcx Should be good to re-review after the latest changes on both the ansible qss side and the tier4_state_rviz_plugin

@xmfcx xmfcx force-pushed the qt5ct-dark-mode-integration branch from 7d024d3 to 2ba16b9 Compare September 12, 2024 09:33
@xmfcx xmfcx marked this pull request as ready for review September 12, 2024 11:06
@xmfcx xmfcx changed the title feat: qt5ct dark mode integration using script feat: add env variable to enable rviz2 theme Sep 12, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Sep 12, 2024

This is just an env var for the theme, we can merge it now. It shouldn't be an issue even if the theme is not installed.

But I will test by removing qt5ct on my machine, just in case.

@xmfcx
Copy link
Contributor

xmfcx commented Sep 12, 2024

I've confirmed by uninstalling qt5ct I was able to run rviz with older theme with this PR enabled.

@xmfcx xmfcx merged commit 42ba6ab into main Sep 12, 2024
21 of 22 checks passed
@xmfcx xmfcx deleted the qt5ct-dark-mode-integration branch September 12, 2024 11:12
@xmfcx xmfcx changed the title feat: add env variable to enable rviz2 theme feat: add an env variable to enable the new rviz2 theme Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants