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

MSA: support dark themes #1173

Merged
merged 7 commits into from Jan 16, 2019

Conversation

Projects
None yet
5 participants
@VictorLamoine
Copy link
Contributor

commented Oct 30, 2018

Description

  • GUI change only. This improves the NavigationWidget in the Setup Assistant for dark themes, without changing anything when using a white theme.
  • Can be cherry-picked but it is not necessary.
  • Tested with Kubuntu 18.04.

The detection of a dark/white theme is done by comparing the luminance value of the widgets background color. QPalette colors are used so that colors always match.

Before

White theme

before_white

Dark theme

before_dark

After (see updated images in the discussion)

White theme

after_white

Dark theme

after_dark

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Document API changes relevant to the user in the moveit/MIGRATION.md notes
  • Created tests, which fail without this PR reference
  • Decide if this should be cherry-picked to other current ROS branches
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers
@rhaschke
Copy link
Collaborator

left a comment

Instead of introducing even more hard-coded colors, I suggest to rely on the theme's palette instead.
The background gradient (if necessary/visible at all) can be achieved by brightening/darkening the palette color.

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Can we agree that if you've reached the point with an OSS project where things like how your GUI looks in certain themes becomes important you deserve a 1.0 release as apparently users have nothing "better" to worry about ;)

@rhaschke rhaschke changed the title Improve navigation widget MSA: support dark themes Nov 11, 2018

@v4hn

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Instead of introducing even more hard-coded colors, I suggest to rely on the theme's palette instead.
The background gradient (if necessary/visible at all) can be achieved by brightening/darkening the palette color.

I would say this is somewhat independent of the requested feature, but nice to have yes.
@rhaschke do you want to hold off the pull-request as long as palette support is not included?

@VictorLamoine

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

I will improve the merge request, I hope to have some time next week to work on this. (next step is doing the same in RViz!)

Instead of hard coded colors we will have hard coded offsets from colors, which is already much better.

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

Instead of hard coded colors we will have hard coded offsets from colors, which is already much better.

+1

Do you want to hold off the pull-request as long as palette support is not included?

Yes. I prefer clean code over a quick fix.

@v4hn

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Yes. I prefer clean code over a quick fix.

This is neither about clean code nor about fixing anything.
It's about hardcoding colors or offsets, as @VictorLamoine stated above.

Thanks for your time @VictorLamoine !

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

IMHO, hardcoding colors is dirty code...
Introducing a second set of hard-coded colors to handle a particular dark scheme is even more dirty and only a quick fix in my opinion. A clean implementation should handle all kinds of color schemes and I support the suggested approach of using offsets.
@VictorLamoine Please ensure to use palette colors as your baseline instead of hard-coded colors ;-)

@VictorLamoine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

I have got a nice result with clean code.
I have ditched some gradients because they would be harder to create/maintain for different themes.

Using http://doc.qt.io/qt-5/qpalette.html roles we can get nice colors.

After

White theme

after_white

Dark theme

after_dark

Tell me what you think!

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Jan 2, 2019

Thanks a lot for this update. The solution is much clearer now.
You should avoid merge commits in pull-requests as this will mess up the commit history of our master branch (at least if we don't squash your commits into a single one). Do you mind to re-commit your changes on top of melodic-devel? To do so, just
git reset --soft origin/melodic-devel
on this PR branch and commit your changes (again). In future, it's better to directly rebase onto the current head:
git rebase origin/melodic-devel

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Jan 2, 2019

I pushed another commit restoring the gradients. Please have a look at the results for a dark theme.

@VictorLamoine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

I have rebased/squashed against latest melodic-devel commit.
It looks good on both themes with the gradients 👍

The de-activated menu doesn't look "great" in my opinion but it is fine:
screenshot_20190103_094824
screenshot_20190103_094838

PS: if you have some time you can also review ros-visualization/rviz#1319 which are similar modifications for RViz

rhaschke added some commits Jan 3, 2019

cleanup
- avoid frequent calls to QApplication::palette()
- use displayText() to format text
- simplify QPoint calculations
- simplify drawing of shadow line
turn widget_instructions_ into QLabel
instead of making the QTextEdit behave like QLabel
@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

After installing qt5-style-plugins, my qt5 applications follow the system style as well...
Using Xubuntu's adwaita-dark theme I observe the same issue as @VictorLamoine with dark texts on dark background. Looks like this is a bug in Qt, because I observe the same behaviour with other Qt applications and standard widgets like LineEdit as well.

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

I fixed some other small issues with theme switching.

@VictorLamoine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

I have tested again and it still looks good on my side (both white / dark themes). Do you want me to squash or do you use GitHub squash and merge?

fixup! cleanup
clang formatting
@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

We will squash-commit if this gets approved.

@v4hn

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

All code changes currently in this request look good to me.
I cannot test this myself at the moment though, so I can't judge the actual appearance.

@v4hn v4hn requested a review from davetcoleman Jan 11, 2019

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

@VictorLamoine I hate to be picky here but I think this is a visual regression of the left side navigation bar for the white theme... when I originally made the MSA I did put effort into making it look good. I'm on Ubuntu 16.04 with Gnome 3 and the standard system colors whatever that is.

Original
image

New White Theme
image

I think the new intense gradient in the navigation is ugly, can you revert that back?

backgroundGradient.setColorAt(0, QColor(245, 245, 245));
backgroundGradient.setColorAt(1, QColor(240, 240, 240));
backgroundGradient.setColorAt(0, palette.color(QPalette::Light));
backgroundGradient.setColorAt(1, palette.color(QPalette::Light).darker(125));

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jan 15, 2019

Member

make this darker amount less?

This comment has been minimized.

Copy link
@rhaschke

rhaschke Jan 15, 2019

Collaborator

@davetcoleman I am to blame here, because I tuned those values.
Of course, that's a matter of taste, but I liked the 3D-look of the gradient. In your original version, the gradient was only barely visible and I suggest to drop the gradient completely if you want to keep it as is (That's what @VictorLamoine did after his rework towards palette colors).
If you like to keep the gradient, however, please play around with the argument to .darker(xxx) yourself to find a value the matches your taste. I actually don't care...

This comment has been minimized.

Copy link
@VictorLamoine

VictorLamoine Jan 15, 2019

Author Contributor

The point of this code change is to make MSA look nice on most platforms, not just on one particular theme or desktop environment so I agree with you Dave.

I will try to tweak those values and test the results on multiple environments to get the best compromise :)

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

I tested it again just now and I think 105 looks good:
image

Also:
image

Update moveit_setup_assistant/src/widgets/navigation_widget.cpp
Co-Authored-By: VictorLamoine <VictorLamoine@users.noreply.github.com>
@VictorLamoine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

screenshot_20190115_173210
screenshot_20190115_173147

Looks good 👍

@rhaschke

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2019

@davetcoleman Do you have any other complaints or is this ready for merging?

@davetcoleman davetcoleman merged commit abc4f40 into ros-planning:melodic-devel Jan 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Thanks!!

@VictorLamoine

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Thank you for your support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.