| mtijink | |
| nicolasfella |
| KDE Connect | |
| VDG |
BUG: 375376
This revision adds dark mode support to the app ( T7044 ). It does so by injecting
theme information into each activity, and making sure that all Views
define their colors by reference to theme attributes.
In order to make this work, all of the buttons with images (like the list
of available devices) now are tinted according to the theme.
While all versions of android support the theme, only devices running
Android ICS or higher will have a toggle button in the drawer.
Open all the screens, both with and without the dark theme on.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Some icons not yet themed correctly:
This should address the pieces I had overlooked (thanks Matthijs!)
I can't apply the patch cleanly, and it seems as if you removed all previous changes, only adding the newest update.
hmm, I'm not entirely convinced.
IMHO orange does not really fit into the dark theme. But I can't think of something better either
The problem is: We use orange for things that need somecontrast to the background (e.g. media player buttons, selected device in the drawer) and for things that don't (e.g. the Actionbar). I would like to see at least the Actionbar dark as well.
Also, the icon of 'Send files' seems darker than the others
The icon used for 'Send files' is atypical - all of the other drawables are at least partially transparent, while that one is opaque. We should probably replace it with a more appropriate asset, possibly in a future change to vector assets.
How does the attached look as a potential dark-mode action bar style?
I think it's fine either way, so whatever @nicolasfella thinks 😃
Regarding the icon: if replacing it with a material (vector) icon looks better, go ahead!
Use true black for window backgrounds, and two darker oranges for the toolbar (0x612e00) and statusbar (0x502600).
To change the diff without losing anything, just use arc diff. You are using arc, right? ;) ) If not, see:
-https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches
My suggestion:
With this patch on top
diff --git a/res/drawable/state_list_drawer_background_dark.xml b/res/drawable/state_list_drawer_background_dark.xml index 0ff668f..bc85bfd 100644 --- a/res/drawable/state_list_drawer_background_dark.xml +++ b/res/drawable/state_list_drawer_background_dark.xml @@ -1,5 +1,5 @@ <?xml version="1.0" encoding="utf-8"?> <selector xmlns:android="http://schemas.android.com/apk/res/android"> - <item android:drawable="@color/primary" android:state_checked="true" /> + <item android:drawable="@color/accent" android:state_checked="true" /> <item android:drawable="@android:color/black" android:state_checked="false" /> </selector> diff --git a/res/layout-v14/nav_dark_mode_switch.xml b/res/layout-v14/nav_dark_mode_switch.xml index 8d5b590..a521b0c 100644 --- a/res/layout-v14/nav_dark_mode_switch.xml +++ b/res/layout-v14/nav_dark_mode_switch.xml @@ -12,7 +12,6 @@ android:paddingStart="16dp" android:paddingTop="4dp" android:text="Dark theme" - android:textColor="@android:color/white" app:switchPadding="12dp" tools:background="@drawable/drawer_header" /> \ No newline at end of file diff --git a/res/layout/nav_header.xml b/res/layout/nav_header.xml index 18f0ed4..fca00a0 100644 --- a/res/layout/nav_header.xml +++ b/res/layout/nav_header.xml @@ -2,7 +2,7 @@ <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" android:layout_width="match_parent" android:layout_height="160dp" - android:background="@drawable/drawer_header" + android:background="@drawable/konqui" android:gravity="bottom" android:orientation="vertical"> @@ -16,7 +16,6 @@ android:paddingStart="16dp" android:paddingTop="8dp" android:text="KDE Connect" - android:textColor="#FFF" android:textStyle="bold" /> <TextView @@ -30,7 +29,6 @@ android:paddingRight="48dp" android:paddingStart="16dp" android:paddingTop="4dp" - android:text="My device" - android:textColor="#fff" /> + android:text="My device" /> </LinearLayout> diff --git a/res/values/styles-dark.xml b/res/values/styles-dark.xml index 5051e15..f0de2f9 100644 --- a/res/values/styles-dark.xml +++ b/res/values/styles-dark.xml @@ -1,7 +1,7 @@ <resources> - <color name="darkToolbarBackground">#612e00</color> - <color name="darkStatusBarBackground">#502600</color> + <color name="darkToolbarBackground">#222222</color> + <color name="darkStatusBarBackground">#333333</color> <!-- KdeConnectThemeBase styles must only be defined in the main res/values/ folder --> <style name="KdeConnectThemeBase.Dark" parent="Theme.AppCompat">
I don't like some of the new changes. I understand this is all opinions, so we can't have everything, but I think that the following things are especially bad changes:
Finally, a remark about black saving energy: KDE Connect is mostly a background app, so saving energy isn't that important. I'd prefer a grey background myself, but I guess other people like black more.
Changing the header is out of scope for this patch and I would suggest doing it at least in a later patch.
Furthermore, if it's done, please make sure we adhere to material design guidelines:
https://material.io/guidelines/patterns/navigation-drawer.html#navigation-drawer-specs
@mtijink I tried to take account of your suggestions. The toolbar and statusbar
now match the colors @nicolasfella proposed, and we have a light grey for the
drawer to establish contrast. I'll upload a screenshot or two as well.
Screenshots from Android 7.1 device:
Medium quality screen-recording from an Android 4.3 Emulator:
