Skip to content

Conversation

@AndyScherzinger
Copy link
Member

@AndyScherzinger AndyScherzinger commented Jan 15, 2019

Resolves #2188
improvement for #3107 (AMOLED, needs a black variant, not a dark grey to improve AMOLED)

dark theme implementation has been done by @dan0xii 🎉

TODO

  • update to material 1.1.0 (when released) Update to material 1.1.0 #4749
  • Login screen has messed up colours on the server address box.
  • "Sharing" search box has black-on-grey text in dark mode. "Sharing" search box has black-on-grey text in dark mode. #4750
  • Rename dialog doesn't match styling of other edittext boxes. Rename dialog doesn't match styling of other edittext boxes. #4751
  • Toggle button background colour is difficult to see.
    I've lightened the track up in dark mode. Example:
    image
  • Share link expiration date picker button text is white-on-white or black-on-grey.
    (Fix by dan0xii although the colours could be tweaked).
  • Issue in settings where theme is not applied immediately (possibly just API 22 and upwards).
    (Possibly fixed. See Dark theme #3459 (comment)).
  • (TBC - dialogs have grey backgrounds) dialogs have a black background - to be discussed. A black dialog on a black activity background can't be distinguished anymore. Maybe we should go for a dark grey tone for the dialogs? (no sure though)
  • Preferences text on settings screen stays black on night theme but should be white instead

Note: Floating action button is black-on-primary colour on API 19.

@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Jan 15, 2019

I have external links on my productive instance, and it seems that they are wrongly colored:
image image

@AndyScherzinger
Copy link
Member Author

I have external links on my productive instance, and it seems that they are wrongly colored:

@tobiasKaminsky yes I also saw that, other than that the PR needs a lot of testing since with the move to AndroidX and supporting day/night via theming I had to abandon the Bridge base themes from the material components, so the theme crashes for legacy AppCompat UI elements (which we shouldn't have in placce anymore, but still), also I can't say if we need to upgrade dialogs, since material components 1.1.0 will introduce new dialog support classes... but well, this is Android dev life...

@nextcloud nextcloud deleted a comment Jan 16, 2019
@nextcloud nextcloud deleted a comment Jan 16, 2019
@dan0xii

This comment has been minimized.

@AndyScherzinger

This comment has been minimized.

@dan0xii

This comment has been minimized.

@AndyScherzinger

This comment has been minimized.

@AndyScherzinger

This comment has been minimized.

@dan0xii

This comment has been minimized.

@AndyScherzinger

This comment has been minimized.

@dan0xii

This comment has been minimized.

@nextcloud nextcloud deleted a comment Jan 17, 2019
@nextcloud nextcloud deleted a comment Jan 17, 2019
@nextcloud nextcloud deleted a comment Jan 18, 2019
@nextcloud nextcloud deleted a comment Jan 18, 2019
@nextcloud nextcloud deleted a comment Jan 18, 2019
@nextcloud nextcloud deleted a comment Jan 18, 2019
@tobiasKaminsky

This comment has been minimized.

@AndyScherzinger

This comment has been minimized.

@AndyScherzinger

This comment has been minimized.

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Oct 28, 2019

@splitt3r thanks for testing, your issues should now be fixed with the latest commits 🚀

@tobiasKaminsky @dan0xii I'd be fine with merging this to master and start with small(er) fixes for papercuts

@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11441.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

Codacy

299

Lint

TypemasterPR
Warnings5963
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

Lint increased!

@nextcloud-android-bot
Copy link
Collaborator

Codacy

299

Lint

TypemasterPR
Warnings5963
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

Lint increased!

@dan0xii
Copy link
Contributor

dan0xii commented Oct 28, 2019

@tobiasKaminsky @dan0xii I'd be fine with merging this to master and start with small(er) fixes for papercuts

Agreed. I'll just continue to try and tidy up the theme to get it to look right and then I'll go back and try and make the code slicker.

Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11443.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

Codacy

299

Lint

TypemasterPR
Warnings5959
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423


Button providerButton = findViewById(R.id.signup);
providerButton.setBackgroundColor(getResources().getColor(R.color.primary_dark));
providerButton.setBackgroundColor(getResources().getColor(R.color.primary));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might lead to problems with branding, please revert.

Canvas c = new Canvas(resultBitmap);

c.drawColor(MainApp.getAppContext().getResources().getColor(R.color.background_color));
c.drawColor(MainApp.getAppContext().getResources().getColor(R.color.bg_default));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change this on demand to dark/light value, this will be permanent in cache and thus might be wrong.
Doing this dynamic would be a way, but for now I think we should just use light background for PNG as always.
(I'll create a ticket to investigate this later).

new int[][]{new int[]{android.R.attr.state_checked},
new int[]{}},
new int[]{trackColor, trackColorUnchecked});
// new int[]{trackColor, Color.parseColor("#4D000000")});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented line

* Base activity with common behaviour for activities dealing with ownCloud {@link Account}s .
*/
public abstract class BaseActivity extends AppCompatActivity implements Injectable {
public abstract class BaseActivity extends AppCompatActivity implements Injectable, SharedPreferences.OnSharedPreferenceChangeListener {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line wrap

* Tracks whether the activity should be recreate()'d after a theme change
*/
private boolean mThemeChangePending;
private boolean mPaused;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no m prefix

@@ -0,0 +1,82 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
ownCloud Android client application
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New license header, as it is a new file


<style name="DatePickerStyle" parent="">
<item name="android:headerBackground">@color/bg_fallback_highlight</item>
<!-- TODO for < API21 -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a real todo? Then please remove it here and create a new issue with label "darkTheme".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed since v16 simply shows a (holo) date picker atm. cc @dan0xii

<color name="black">#000000</color>
<color name="white">#FFFFFF</color>
<color name="textColor">@color/black</color>
<!--<color name="white">#FFFFFF</color>-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

<string name="prefs_imprint">Imprint</string>
<string name="prefs_value_theme_light">Light</string>
<string name="prefs_value_theme_dark">Dark</string>
<string name="prefs_key_theme">darkTheme</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a preference internal key, please move it

@@ -1,5 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
<?xml version="1.0" encoding="utf-8"?><!--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 2 lines (though I know that Android Studio likes to put this into one line…)

@tobiasKaminsky
Copy link
Member

Only minor changes due to code review.
@dan0xii @AndyScherzinger this is an amazing work! 🎉 🎉 🎉 🎉

Once it is merged, I'll create some issues so that we can fix this in smaller steps.

@AndyScherzinger
Copy link
Member Author

taking care of the review comments right now

Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
@AndyScherzinger
Copy link
Member Author

@tobiasKaminsky code review changes are ✔️ DONE

Feel free to re-review, test-drive and merge :)

@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11455.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud nextcloud deleted a comment Oct 29, 2019
@nextcloud-android-bot
Copy link
Collaborator

Codacy

299

Lint

TypemasterPR
Warnings5959
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings69
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings44
Dodgy code Warnings138
Total423

@AndyScherzinger
Copy link
Member Author

Hi everyone,
the initial work has been merged to master an will be paart of tomorrow's dev build. All further findings have been labeled feature: theming

@dan0xii
Copy link
Contributor

dan0xii commented Oct 29, 2019

So glad to get this merged. Thanks for the help getting it over the line, @AndyScherzinger.

tobiasKaminsky added a commit that referenced this pull request Oct 30, 2019
2a43da6 Merge pull request #4746 from nextcloud/createChooser
f8d7ddd Merge pull request #4758 from nextcloud/fixDarkTheme
bee8770 - fix typo in styles - fix sign up with provider in dark primary color
cc710b0 Merge pull request #3459 from nextcloud/dark_theme
c1aa005 changes due to code review
c801a2c Merge pull request #4744 from nextcloud/v3100_Alpha1
12fe91a use default open chooser with "just once", "always"
8b4481f [tx-robot] updated from transifex
2ea1f4e daily dev 20191029
@tobiasKaminsky
Copy link
Member

And this is the link: https://github.com/nextcloud/android/issues?q=is%3Aopen+is%3Aissue+label%3A%22feature%3A+theming%22

Some are bugs, some are enhancements :-)

@AndyScherzinger
Copy link
Member Author

So glad to get this merged. Thanks for the help getting it over the line, @AndyScherzinger.

Always a pleasure to help @dan0xii ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dark theme

8 participants