Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue #7633 #7664

Merged
merged 3 commits into from
Dec 17, 2020
Merged

Fix issue #7633 #7664

merged 3 commits into from
Dec 17, 2020

Conversation

czlucius
Copy link
Contributor

@czlucius czlucius commented Dec 14, 2020

Fix for issue: #7633
Add ThemedListPreference, extending ListPreference, to theme dialog created by ListPreference.
Add style for dialog, setting default button text color in fallback dialog theme to ?android:attr/textColorPrimary

Testing

Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.

unit tests
instrumented tests
UI tests

  • Tests written, or not not needed

How to do a test? I am new to contributing to open source, and I noticed that this issue is labelled with "good-first-issue". Apologies if this PR is not well written/there are problems with this commit.

@AndyScherzinger
Copy link
Member

@czlucius could you post a screenshot please? ❤️ Looking at the code shouldn't it be enough to have a custom alertDialogTheme added to the fallback theme that takes care of the styling? (just by looking at https://stackoverflow.com/questions/51049516/how-to-change-the-style-of-a-listpreference-popup-dialog)

@czlucius
Copy link
Contributor Author

Screenshot_20201215-123302_Nextcloud.jpg

@czlucius
Copy link
Contributor Author

@czlucius could you post a screenshot please? Looking at the code shouldn't it be enough to have a custom alertDialogTheme added to the fallback theme that takes care of the styling? (just by looking at https://stackoverflow.com/questions/51049516/how-to-change-the-style-of-a-listpreference-popup-dialog)

Yes. I did not look into this option. I shall modify the code.
Thanks

@czlucius
Copy link
Contributor Author

By the way, owncloud.AlertDialog is already the alertDialogTheme.

@AndyScherzinger
Copy link
Member

That one is used to theme the dialogs other than the ones launched via the settings screen (where we have full control on programmatically theming the dialog on-the-fly based on server-side colors. Since that doesn't work nicely for settings when introduced so calls "FallbackThemes" so you might create a grey-ish fallback dialog theme.

@AndyScherzinger
Copy link
Member

@czlucius any reason for closing the PR? 😢

@czlucius czlucius reopened this Dec 16, 2020
@czlucius
Copy link
Contributor Author

Sorry, I had resetted my previous commit, so I had to force push my local repo to GitHub.

@czlucius
Copy link
Contributor Author

I think GitHub automatically closed it when I had reset my previous commit

@czlucius
Copy link
Contributor Author

That one is used to theme the dialogs other than the ones launched via the settings screen (where we have full control on programmatically theming the dialog on-the-fly based on server-side colors. Since that doesn't work nicely for settings when introduced so calls "FallbackThemes" so you might create a grey-ish fallback dialog theme.

Ok. But why does the button colors not change when I modify FallbackTheming.Dialog and add a button style? I had to modify owncloud.AlertDialog for it to work.

@AndyScherzinger
Copy link
Member

Ok. But why does the button colors not change when I modify FallbackTheming.Dialog and add a button style? I had to modify owncloud.AlertDialog for it to work.

I think you need to add the dialog theme to the FallBackTheme

@czlucius
Copy link
Contributor Author

I updated the fallback theme(FallbackThemingThemeBase) to include FallbackTheming.Dialog as the alert dialog theme, and the cancel button still stays invisible.

Right now, I have found 2 solutions:

  • Use a custom ListPreference to launch alert dialog with custom theme
  • Update owncloud.AlertDialog's alertDialogTheme to FallbackTheming.Dialog.

@AndyScherzinger
Copy link
Member

Wouldn't the later change all other dialogs within the app too? Without having had a look and giving it a try (I am away from my laptop) I'd say the first solution would be save(er) 👍

@AndyScherzinger
Copy link
Member

Forgot to mention: thanks for all your effort and patience discussing the solution. This is highly appreciated ❤️

@czlucius
Copy link
Contributor Author

Wouldn't the later change all other dialogs within the app too? Without having had a look and giving it a try (I am away from my laptop) I'd say the first solution would be save(er)

You may want to see the styles.xml here: https://pastebin.com/QzS8ipY4

@czlucius
Copy link
Contributor Author

Forgot to mention: thanks for all your effort and patience discussing the solution. This is highly appreciated ❤️

Thanks

@czlucius
Copy link
Contributor Author

image
I noticed SettingsActivity uses Theme.ownCloud as the theme instead of the fallback theme. Theme.ownCloud has an alert dialog attribute of ownCloud.Dialog.

@czlucius
Copy link
Contributor Author

Perhaps that is why SettingsActivity's dialogs follow ownCloud.Dialog instead of FallbackTheming.Dialog?

@AndyScherzinger
Copy link
Member

Yeah, that might be the root cause, could you check if it "works" if you switch it to the fallback theme?

@czlucius
Copy link
Contributor Author

I updated the fallback theme(FallbackThemingThemeBase) to include FallbackTheming.Dialog as the alert dialog theme, and the cancel button still stays invisible.

Right now, I have found 2 solutions:

  • Use a custom ListPreference to launch alert dialog with custom theme
  • Update owncloud.AlertDialog's alertDialogTheme to FallbackTheming.Dialog.

Which solution would be the best?

@czlucius
Copy link
Contributor Author

Yeah, that might be the root cause, could you check if it "works" if you switch it to the fallback theme?

I have tried switching the theme, but the button stays invisible.

@czlucius
Copy link
Contributor Author

I think the custom ListPreference is still the best option.

@AndyScherzinger
Copy link
Member

@czlucius then let's go with your solution 👍

@czlucius
Copy link
Contributor Author

Ok. But why does the button colors not change when I modify FallbackTheming.Dialog and add a button style? I had to modify owncloud.AlertDialog for it to work.

I think you need to add the dialog theme to the FallBackTheme

Sorry. I made a mistake in the styles.xml and used buttonBarButtonStyle instead of android:buttonBarButtonStyle. I have tested it and it turns out that the dialog's buttons can be styled, just by adding android:buttonBarButtonStyle to the FallbackTheming.Dialog theme. I think the custom ListPreference won't be needed then, as this is a simpler solution.

Add `android:buttonBarButtonStyle` to `FallbackTheming.Dialog`, modifying the button style of the fallback `AlertDialog`, so that the button would not appear as invisible.
This commit changes the button text color from black(#000000) to a prominent text color that varies depending on the theme settings.
@czlucius
Copy link
Contributor Author

I have pushed the updates to this PR, you may take a look at the commits made.
Right now, there is a slight problem with using ?android:attr/textColorPrimary, because if the theme in the app settings was set to always dark, the text color would still follow the system(system light/dark mode) and not remain white color in Android's light mode, when the app theme is set to dark.

This commit changes the text color of the AlertDialog's buttons from `?android:attr/textColorPrimary` to `@color/text_color`.
This ensures that the button text can be readable throughout all themes.
@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

Codacy

Lint

TypemasterPR
Warnings271271
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings52
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings41
Dodgy code Warnings99
Total310

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings52
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings41
Dodgy code Warnings99
Total310

Copy link
Member

@AndyScherzinger AndyScherzinger left a comment

Choose a reason for hiding this comment

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

@czlucius with your latest commit it works like a charm 👍 @color/text_color is doing it's job 🎉

@AndyScherzinger AndyScherzinger marked this pull request as ready for review December 17, 2020 21:48
@AndyScherzinger AndyScherzinger merged commit 03606f0 into nextcloud:master Dec 17, 2020
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.15.0 milestone Dec 17, 2020
@czlucius
Copy link
Contributor Author

@czlucius with your latest commit it works like a charm 👍 @color/text_color is doing it's job 🎉

Thanks

@czlucius
Copy link
Contributor Author

By the way, while testing, I keep on receiving crashes with this exception while opening the app


2020-12-17 02:55:46.969 31240-32376/com.nextcloud.client E/AndroidRuntime: FATAL EXCEPTION: Thread-14
    Process: com.nextcloud.client, PID: 31240
    java.lang.NoClassDefFoundError: Failed resolution of: Lokio/Buffer;
        at okhttp3.ResponseBody.create(ResponseBody.java:210)
        at okhttp3.internal.Util.<clinit>(Util.java:62)
        at okhttp3.Credentials.basic(Credentials.java:30)
        at com.owncloud.android.lib.common.OwnCloudClientFactory.createNextcloudClient(OwnCloudClientFactory.java:217)
        at com.owncloud.android.operations.RefreshFolderOperation.updatePredefinedStatus(RefreshFolderOperation.java:330)
        at com.owncloud.android.operations.RefreshFolderOperation.updateCapabilities(RefreshFolderOperation.java:304)
        at com.owncloud.android.operations.RefreshFolderOperation.updateOCVersion(RefreshFolderOperation.java:276)
        at com.owncloud.android.operations.RefreshFolderOperation.run(RefreshFolderOperation.java:229)
        at com.owncloud.android.lib.common.operations.RemoteOperation.run(RemoteOperation.java:360)
        at java.lang.Thread.run(Thread.java:919)
     Caused by: java.lang.ClassNotFoundException: Didn't find class "okio.Buffer" on path: DexPathList[[zip file "/data/app/com.nextcloud.client-f8CDsZTvR0Kck4sHwWuMLw==/base.apk"],nativeLibraryDirectories=[/data/app/com.nextcloud.client-f8CDsZTvR0Kck4sHwWuMLw==/lib/arm64, /data/app/com.nextcloud.client-f8CDsZTvR0Kck4sHwWuMLw==/base.apk!/lib/arm64-v8a, /system/lib64, /product/lib64]]
        at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:196)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:379)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:312)
        at okhttp3.ResponseBody.create(ResponseBody.java:210) 
        at okhttp3.internal.Util.<clinit>(Util.java:62) 
        at okhttp3.Credentials.basic(Credentials.java:30) 
        at com.owncloud.android.lib.common.OwnCloudClientFactory.createNextcloudClient(OwnCloudClientFactory.java:217) 
        at com.owncloud.android.operations.RefreshFolderOperation.updatePredefinedStatus(RefreshFolderOperation.java:330) 
        at com.owncloud.android.operations.RefreshFolderOperation.updateCapabilities(RefreshFolderOperation.java:304) 
        at com.owncloud.android.operations.RefreshFolderOperation.updateOCVersion(RefreshFolderOperation.java:276) 
        at com.owncloud.android.operations.RefreshFolderOperation.run(RefreshFolderOperation.java:229) 
        at com.owncloud.android.lib.common.operations.RemoteOperation.run(RemoteOperation.java:360) 
        at java.lang.Thread.run(Thread.java:919) 

I had to turn off the Wi-Fi so that this error will not happen

Steps to reproduce:

  • Turn on Wi-Fi or mobile data, connected to the Internet
  • Open the app, signed in to an account
  • Pull to refresh/Tap the switch accounts button

Device info:
Samsung Galaxy Tab S6 Lite(SM-P610)
Android 10(API 29)

thelittlefireman pushed a commit to thelittlefireman/android that referenced this pull request Mar 19, 2021
Fix issue nextcloud#7633
Signed-off-by: thelittlefireman <thelittlefireman@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants