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

Support dark mode #769

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support dark mode #769

wants to merge 4 commits into from

Conversation

A6GibKm
Copy link
Contributor

@A6GibKm A6GibKm commented Aug 5, 2022

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Aug 5, 2022

I just realized that the mouse has to be drawn in a way that accommodates dark mode, marking as draft.

@A6GibKm A6GibKm marked this pull request as draft August 5, 2022 08:23
@staticssleever668
Copy link
Member

It already follows the system GTK theme, so does this make any useful difference?
If I understand correctly, this also makes it follow the Freedesktop'ssetting, but are there desktop environments that expose this setting but not the GTK one? Something on based on Qt and in Wayland, perhaps?
For reference, I use following ways to set the theme without using a desktop environment on X11:

  • GTK 3: gtk-application-prefer-dark-theme and gtk-theme-name settings in $XDG_CONFIG_HOME/gtk-3.0/settings.ini.
  • xsettingsd: Net/ThemeName in $XDG_CONFIG_HOME/xsettingsd/xsettingsd.conf.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Aug 5, 2022

The system GTK theme is not a supported way to set color schemes, this makes it so that the setting can be applied on GNOME 42 or later when using the Appearance panel in GNOME Settings and other desktop environments who support the xdg settings portal (KDE does for example).

If you use GTK_THEME or similar then this will overwrite whatever the portal says so it will still do what you wanted.

Note that using HdyStyleManager one is able to react to the theme and listen for changed and one can think in doing things like displaying the mouse svg in grey for example. This wasn't possible with env vars or older methods.

EDIT: I added a commit to draw the svg in a more appropriate color, but I have no clue whats the correct css here. Maybe it is not even possible to do so?

EDIT: I wasn't specific enough, the setting exposed in modern GNOME desktops will NOT set GTK_THEME or any similar flag, hence this app will appear as white no matter what if the user has dark mode in settings but hasn't manually set GTK_THEME or similar. Similar implementations (Elementary for example) might do the same.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Aug 5, 2022

You can find more info at https://gitlab.gnome.org/GNOME/Initiatives/-/issues/32.

@whot
Copy link
Member

whot commented Aug 15, 2022

added a commit to draw the svg in a more appropriate color, but I have no clue whats the correct css here. Maybe it is not even possible to do so?

@jimmac was the one to suggest this particular style of SVG, so best is probably to get some guidance here for colors in dark mode.

@jimmac
Copy link

jimmac commented Sep 30, 2022

I've filed an issue for the artwork and have been grinding on the dark variants as well as cleaning up the style bit.

@jimmac
Copy link

jimmac commented Nov 21, 2022

jimmac@823fcf9 shows the newly included dark assets also involved some fixes to the light assets. Would be nice to have the assets part of this pull request and tested working.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Nov 23, 2022

@jimmac thanks a lot!

image

@whot The PR is ready for review again. By the way, I used the walrus operator (:=) bumping the required python version to 3.7.

@A6GibKm A6GibKm force-pushed the color-scheme branch 2 times, most recently from 409827d to 2ea8ff9 Compare November 23, 2022 12:38
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Nov 23, 2022

There are some errors in flake8 and check_svg but I am not sure where are they coming from.

@A6GibKm A6GibKm marked this pull request as ready for review November 23, 2022 13:25
@staticssleever668
Copy link
Member

@A6GibKm, you can the error by clicking details on the failed action, and then selecting View raw logs in the menu of the button that looks like a gear.
Also you can run the tests locally: meson test -Cbuild.

The failing flake8 test runs without an error for me locally.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Nov 23, 2022

The failing flake8 test runs without an error for me locally.

same.

Also you can run the tests locally: meson test -Cbuild.

I get No tests defined. for some reason.

@staticssleever668
Copy link
Member

I get No tests defined. for some reason.

You probably configured the project with tests disabled, I suggest you change the option:
meson configure ./build -D tests=true; or setup the project from scratch: rm -r build && meson setup build.

Also, sorry, the build directory is builddir in Piper's docs, if you follow them, replace build in these commands with builddir, e.g. meson test -C builddir.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Nov 23, 2022

I mean these are a few lines from the output:

INFO:/home/runner/work/piper/piper/data/svgs/logitech-g-pro-x-wireless-superlight.svg:Found 0 leds
INFO:/home/runner/work/piper/piper/data/svgs/logitech-mx-anywhere3.svg:Found 6 buttons
INFO:/home/runner/work/piper/piper/data/svgs/logitech-mx-anywhere3.svg:Found 0 leds
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-senseiten-dark.svg:Found 7 buttons
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-senseiten-dark.svg:Missing led0-leader for led0
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-senseiten-dark.svg:Missing led0-path for led0
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-senseiten-dark.svg:Found 1 leds
ERROR:/home/runner/work/piper/piper/data/svgs/logitech-g500.svg:Have button2-leader but not button2
WARNING:/home/runner/work/piper/piper/data/svgs/logitech-g500.svg:Non-consecutive button: button3
INFO:/home/runner/work/piper/piper/data/svgs/logitech-g500.svg:Found 10 buttons
INFO:/home/runner/work/piper/piper/data/svgs/logitech-g500.svg:Found 0 leds
INFO:/home/runner/work/piper/piper/data/svgs/logitech-m720.svg:Found 8 buttons
INFO:/home/runner/work/piper/piper/data/svgs/logitech-m720.svg:Found 0 leds
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v3-dark.svg:Found 4 buttons
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v3-dark.svg:Missing led0-leader for led0
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v3-dark.svg:Missing led0-path for led0
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v3-dark.svg:Found 1 leds
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-rival310-dark.svg:Found 6 buttons
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-rival310-dark.svg:Have led1-leader but not led1
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-rival310-dark.svg:Found 1 leds
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v2-dark.svg:Found 4 buttons
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v2-dark.svg:Missing led0-leader for led0
ERROR:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v2-dark.svg:Missing led0-path for led0
INFO:/home/runner/work/piper/piper/data/svgs/steelseries-kinzu-v2-dark.svg:Found 1 leds
INFO:/home/runner/work/piper/piper/data/svgs/logitech-g402-dark.svg:Found 8 buttons
INFO:/home/runner/work/piper/piper/data/svgs/logitech-g402-dark.svg:Found 2 leds

are those errors and warnings the reason it is failing? if so, it is because I didn't adapt the test or because the svgs might lack something?

@staticssleever668
Copy link
Member

I don't see any changes to the code that would need adaptation of the tests, so it's probably the problem with svgs.

@whot
Copy link
Member

whot commented Nov 24, 2022

Missing led0-leader for led0

The line pointing to led0 doesn't have the id led0-leader. That's usually a problem with the SVG. See data/svg/README.md for details on the SVG requirements

I filed a PR to bump the CI to Ubuntu 22.04 (#806), maybe that fixes the flake8 issues.

jimmac and others added 4 commits November 25, 2022 12:17
- dark theme preference covered by `-dark.svg` assets
- use colors based on the [gnome icon
  palette](https://developer.gnome.org/hig/reference/palette.html)

See libratbag#792
This bundles the correct libhandy
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Nov 25, 2022

Yeah, that fixed the flake8 side.

@whot
Copy link
Member

whot commented Nov 25, 2022

I used the walrus operator (:=) bumping the required python version to 3.7.

just saw that, a bit late but then again, I have the great excuse of having been away :)

please don't bump the required version. IIRC 18.04 lts is still on 3.6 and the walrus operator alone isn't really enough of a feature to justify it. Also, quick google suggests the walrus was introduced in 3.8

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Nov 26, 2022

Ok, fair enough, ill remove that.

@qwertychouskie
Copy link

I used the walrus operator (:=) bumping the required python version to 3.7.

just saw that, a bit late but then again, I have the great excuse of having been away :)

please don't bump the required version. IIRC 18.04 lts is still on 3.6 and the walrus operator alone isn't really enough of a feature to justify it. Also, quick google suggests the walrus was introduced in 3.8

Ubuntu 18.04 LTS is EOL now, so probably not worth holding back this PR.

Would be nice to see this merged :)

@qwertychouskie
Copy link

Is there anything holding this PR back from being merged? (If it's helpful, I could open up a rebased PR if @A6GibKm doesn't have the time to rebase it.)

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Oct 22, 2024

I can rebase today, but I do remember there was something fishy somewhere (perhaps fixed by the latest changes).

@jimmac
Copy link

jimmac commented Oct 22, 2024

In the quest to fix the graphic assets stylistically and keep a somewhat consistent canvas size, I've touched pretty much every single asset. While they are now great from the stylistic point of view now, I broke some of the assets in the technical aspect. The svgs depend on certain structure for the element hightlighting to work. So this requires going through the whole set and fix those warnings.

However it's a moving target and very likely more assets have been introduced in the meantime. I really ran out of energy for this the whack-a-mole game :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants