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

Fixed #158(scrollbar issue) #309

Merged
merged 14 commits into from
Aug 4, 2023
Merged

Fixed #158(scrollbar issue) #309

merged 14 commits into from
Aug 4, 2023

Conversation

StellarSand
Copy link
Contributor

@StellarSand StellarSand commented Jun 29, 2023

  • Fixed Problem with scrollbar #158
    I've added // TODO comments in ThemeDialogFragment and ExodusApplication classes, so you can get the checked item value from preference and set it there.

Copy link
Contributor

@Jean-BaptisteC Jean-BaptisteC left a comment

Choose a reason for hiding this comment

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

@StellarSand
Copy link
Contributor Author

Done. ✔️
P.S: mind if I replace relative layout elements with the better constraint layouts?

@Jean-BaptisteC
Copy link
Contributor

P.S: mind if I replace relative layout elements with the better constraint layouts?

Ok to improve layout but this changes have an impact on differents configurations layouts ?
Because we have planned to add a new layout for tablet #296

@StellarSand
Copy link
Contributor Author

Just like the fragment_about.xml we can add multiple different layouts,

  • one normal for portrait mode
  • another in res/layout-w600dp (For 7” tablets, 600dp)
  • another in res/layout-w720dp (For 10” tablets 720dp)
    and so on?

@Jean-BaptisteC
Copy link
Contributor

Ok, but add screenshots foreach config to see difference

@StellarSand
Copy link
Contributor Author

There won't be any changes visually. UI will remain as it is right now. Just changes in xml layouts code. Later when we add multiple layouts for tablets, things can be re-arranged for tablet specific views like you showed in the screenshot in #296

@Jean-BaptisteC
Copy link
Contributor

Ok, go on

@StellarSand
Copy link
Contributor Author

Done. All nested layouts have been flattened.

@Jean-BaptisteC
Copy link
Contributor

  • Please reduce space between elements in App Fragment (Same space as app in trackerdetails Fragment
  • Sub permission Fragment is broken, exodus show only 10 permissions, and can you reduce space between elements
  • Percentage bar in tracker Fragment is broken, when percentage is 100%, bar is cut at the right of the screen

@StellarSand
Copy link
Contributor Author

  • Please reduce space between elements in App Fragment

Done

  • Sub permission Fragment is broken, exodus show only 10 permissions, and can you reduce space between elements

Hmm, for me it started showing 11 items. couldn't figure out why, so reverted this back to relative layout.

  • Percentage bar in tracker Fragment is broken, when percentage is 100%, bar is cut at the right of the screen

In android studio the layout is being rendered correctly. I already added marginEnd=30dp and it showed ample amount of space on the right. Didn't even reach the > on the right. But when testing on device, progressbar doesn't seem to respect margin values. So reverting this layout too.

@Jean-BaptisteC
Copy link
Contributor

  • Reduce space between appAVTV and appAnalyzedVersionTV in app_details_fragment
  • Please check com.google.android.material.imageview.ShapeableImageView in recycler view app item and fragment app details, apps icon are cut in somes cases

I have start tests on theming:

  • Can you add checkbox or switch to enable OLED mode on comptatible devices ?
  • Change theming doesn't work on android 5.0

@StellarSand
Copy link
Contributor Author

StellarSand commented Jun 30, 2023

  • Reduce space between appAVTV and appAnalyzedVersionTV in app_details_fragment

  • Please check com.google.android.material.imageview.ShapeableImageView in recycler view app item and fragment app details, apps icon are cut in somes cases

fixed

@Jean-BaptisteC
Copy link
Contributor

Layout looks good

@StellarSand
Copy link
Contributor Author

StellarSand commented Jun 30, 2023

  • Can you add checkbox or switch to enable OLED mode on comptatible devices ?

Unfortunately, I don't know how to. Since there is no inbuilt method for this, we probably have to define a custom style with all the colors (background, status bar, nav bar, fab everything). When user selects checkbox/switch we'd have to set the theme/style to our custom oled style. When user unchecks, we'd again set it back to base theme. That's the idea at least, but last time I tried this, it didn't work, so 🤷

  • Change theming doesn't work on android 5.0

Maybe we show the theme option only for sdk > 21? 🤔
According to #283 support for sdk 21 & 22 will be dropped soon any way.

@Jean-BaptisteC
Copy link
Contributor

Jean-BaptisteC commented Jul 1, 2023

I have check on others Android apps, to detect OLED mode, apps check configuration -> https://developer.android.com/develop/ui/views/theming/darktheme#java
For OLED theme, apply dark theme with black background (not need create a new style)

I used apps with dark theme in API 21 (NewPipe, Organic Maps, VLC), i think it's doesn't work because, we don't have Base.V21 Theme in our styles

@StellarSand
Copy link
Contributor Author

StellarSand commented Jul 1, 2023

  • For OLED theme, apply dark theme with black background (not need create a new style)

  • we don't have Base.V21 Theme in our styles

okay it's done. You can do the // TODO stuff and it will be fully working.

@Jean-BaptisteC
Copy link
Contributor

I'm not a developer, i try to maintain project but i'm not expert.
I can give you somes help to finish Theme option.
Can you create separate PR to theme option, i want merge scrollbar fix.

I have noticed somes things, on theme option:

  • Android top bar and bottom bar doesn't respect colorPrimary in Android 21
  • I we want add oled mode, you need to add OK button to validate selection

@StellarSand
Copy link
Contributor Author

Can you create separate PR to theme option, i want merge scrollbar fix.

Yeah sure no problem. :)

* I we want add oled mode, you need to add `OK` button to validate selection

Why 🤔 You just check the checkbox and it switches to OLED mode directly and vice versa.

@Jean-BaptisteC
Copy link
Contributor

On other app, to enable oled mode, we need to select dark mode before enable oled

@StellarSand
Copy link
Contributor Author

StellarSand commented Jul 6, 2023

Yeah same here too. The OLED mode will only be enabled if dark mode is selected, or if it's follow system and the system theme is dark mode. I've handled that part already. All that remains is the // TODO stuff to set the checked values in preferences.

@Jean-BaptisteC
Copy link
Contributor

In somes cases, if i want enable OLED mode.
I select dark mode in pop-up -> pop-up is closed
I need to re-open pop-up to select OLED mode.

@StellarSand
Copy link
Contributor Author

Hmm, okay I'll do it when I split the theme stuff into a new pull request. Will do in a couple of days.

@StellarSand
Copy link
Contributor Author

Reverted theme stuff so it now only has scrollbar fixes and layout changes.
Here's the separate theme PR: #321

@Jean-BaptisteC
Copy link
Contributor

Thanks, can you remove commits about theming ?

StellarSand and others added 11 commits August 4, 2023 16:50
Bumps [org.jetbrains.kotlinx:kotlinx-coroutines-android](https://github.com/Kotlin/kotlinx.coroutines) from 1.7.1 to 1.7.2.
- [Release notes](https://github.com/Kotlin/kotlinx.coroutines/releases)
- [Changelog](https://github.com/Kotlin/kotlinx.coroutines/blob/master/CHANGES.md)
- [Commits](Kotlin/kotlinx.coroutines@1.7.1...1.7.2)

---
updated-dependencies:
- dependency-name: org.jetbrains.kotlinx:kotlinx-coroutines-android
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
#311)

Bumps [org.jetbrains.kotlinx:kotlinx-coroutines-test](https://github.com/Kotlin/kotlinx.coroutines) from 1.7.1 to 1.7.2.
- [Release notes](https://github.com/Kotlin/kotlinx.coroutines/releases)
- [Changelog](https://github.com/Kotlin/kotlinx.coroutines/blob/master/CHANGES.md)
- [Commits](Kotlin/kotlinx.coroutines@1.7.1...1.7.2)

---
updated-dependencies:
- dependency-name: org.jetbrains.kotlinx:kotlinx-coroutines-test
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jean-Baptiste <[email protected]>
Bumps `hilt_version` from 2.46.1 to 2.47.

Updates `com.google.dagger:hilt-compiler` from 2.46.1 to 2.47
- [Release notes](https://github.com/google/dagger/releases)
- [Changelog](https://github.com/google/dagger/blob/master/CHANGELOG.md)
- [Commits](google/dagger@dagger-2.46.1...dagger-2.47)

Updates `com.google.dagger:hilt-android` from 2.46.1 to 2.47
- [Release notes](https://github.com/google/dagger/releases)
- [Changelog](https://github.com/google/dagger/blob/master/CHANGELOG.md)
- [Commits](google/dagger@dagger-2.46.1...dagger-2.47)

---
updated-dependencies:
- dependency-name: com.google.dagger:hilt-compiler
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: com.google.dagger:hilt-android
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [com.google.dagger.hilt.android](https://github.com/google/dagger) from 2.46.1 to 2.47.
- [Release notes](https://github.com/google/dagger/releases)
- [Changelog](https://github.com/google/dagger/blob/master/CHANGELOG.md)
- [Commits](google/dagger@dagger-2.46.1...dagger-2.47)

---
updated-dependencies:
- dependency-name: com.google.dagger.hilt.android
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jean-Baptiste <[email protected]>
Bumps [com.google.dagger:hilt-android-testing](https://github.com/google/dagger) from 2.46.1 to 2.47.
- [Release notes](https://github.com/google/dagger/releases)
- [Changelog](https://github.com/google/dagger/blob/master/CHANGELOG.md)
- [Commits](google/dagger@dagger-2.46.1...dagger-2.47)

---
updated-dependencies:
- dependency-name: com.google.dagger:hilt-android-testing
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jean-Baptiste <[email protected]>
#320)

Bumps [org.jetbrains.kotlinx:kotlinx-coroutines-test](https://github.com/Kotlin/kotlinx.coroutines) from 1.7.2 to 1.7.3.
- [Release notes](https://github.com/Kotlin/kotlinx.coroutines/releases)
- [Changelog](https://github.com/Kotlin/kotlinx.coroutines/blob/master/CHANGES.md)
- [Commits](https://github.com/Kotlin/kotlinx.coroutines/commits)

---
updated-dependencies:
- dependency-name: org.jetbrains.kotlinx:kotlinx-coroutines-test
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [org.jetbrains.kotlinx:kotlinx-coroutines-android](https://github.com/Kotlin/kotlinx.coroutines) from 1.7.2 to 1.7.3.
- [Release notes](https://github.com/Kotlin/kotlinx.coroutines/releases)
- [Changelog](https://github.com/Kotlin/kotlinx.coroutines/blob/master/CHANGES.md)
- [Commits](https://github.com/Kotlin/kotlinx.coroutines/commits)

---
updated-dependencies:
- dependency-name: org.jetbrains.kotlinx:kotlinx-coroutines-android
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@StellarSand
Copy link
Contributor Author

okay I'm pretty sure I removed the correct commits relating to theme. Please check & confirm.

@StellarSand StellarSand changed the title Fixed #158(scrollbar issue) and #239(Theme option) Fixed #158(scrollbar issue) Aug 4, 2023
Copy link
Contributor

@Jean-BaptisteC Jean-BaptisteC left a comment

Choose a reason for hiding this comment

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

✅ Pixel 6 - Android 13

@Jean-BaptisteC Jean-BaptisteC merged commit 6251bb6 into Exodus-Privacy:master Aug 4, 2023
1 check passed
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.

Problem with scrollbar
2 participants