autowarefoundation / autoware_launch

Apache License 2.0
29 stars 261 forks source link

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

Closed KhalilSelyan closed 3 weeks ago

KhalilSelyan commented 4 months ago

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

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.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

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

xmfcx commented 4 months ago

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.

KhalilSelyan commented 4 months ago

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 https://github.com/autowarefoundation/autoware/pull/4838

stale[bot] commented 2 months ago

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

xmfcx commented 1 month ago

image

It looks like this for me, is this expected?

The button in the circled area looks weird for me.

xmfcx commented 1 month ago

image

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

reference: 2024-09-05_15-15

xmfcx commented 3 weeks ago

image

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

image

xmfcx commented 3 weeks ago

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 commented 3 weeks ago

image

image

this has issues

xmfcx commented 3 weeks ago

image

image

disabled things don't look disabled

xmfcx commented 3 weeks ago

image

image

xmfcx commented 3 weeks ago

image

when selected normally, paddings are ok.

when clicked, normal rviz looks ok: image

but when selected on new theme, it moves right and the down button is clipped a bit: image and also has rounding, it shouldn't have.

when menu comes, rviz is consistent: image

but new theme: image

hovered items should follow the menu item colors as in any other menu items. menu background too. and since this is not rounded, everything else related to this menu should also not be rounded. also its checkbox and text should have the same highlight.

xmfcx commented 3 weeks ago

image

qmenubar colors are wrong and not readable (contrast issues) when hovered

xmfcx commented 3 weeks ago

image

please refer to the tab building blocks from the figma page while styling these tabs, selected and hovered are the same right now.

image

xmfcx commented 3 weeks ago

image

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

xmfcx commented 3 weeks ago

image

image

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

KhalilSelyan commented 3 weeks ago

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

xmfcx commented 3 weeks ago

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 commented 3 weeks ago

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