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

Refactor to AndroidX #172

Merged
merged 20 commits into from
Jul 12, 2018
Merged

Refactor to AndroidX #172

merged 20 commits into from
Jul 12, 2018

Conversation

Mygod
Copy link
Contributor

@Mygod Mygod commented Jul 11, 2018

Fix #146. TODO:

  • Rename to com.takisoft.preferencex;
  • BUG: Scrollbars in SeekbarPreference?

Continuing discussion in #171, I think it's best to rename this repo with redirection and keep the old code in another branch. Also I'd like to change version scheme if you don't mind (see 2549217).

@Mygod Mygod changed the title Refactor to Androidx Refactor to AndroidX Jul 12, 2018
@Mygod
Copy link
Contributor Author

Mygod commented Jul 12, 2018

@Gericop Ready to merge.

@gregkorossy gregkorossy merged commit ea63a45 into gregkorossy:dev-v28 Jul 12, 2018
@gregkorossy
Copy link
Owner

Going to test it thoroughly and hopefully release it tomorrow to bintray.

@Mygod
Copy link
Contributor Author

Mygod commented Jul 13, 2018

Any updates?

@gregkorossy
Copy link
Owner

Working on it right now but my Android Studio is being an idiot. First it launched the app which crashed due to "missing MyApplication" class (wtf? it's there...) and now it just wouldn't recognize the project as an Android one anymore...

@gregkorossy
Copy link
Owner

At least the bintray repo is set up at https://bintray.com/takisoft/android.

@Mygod
Copy link
Contributor Author

Mygod commented Jul 13, 2018

Awesome! You should try clean project + invalidate cache and restart. 🤣

@gregkorossy
Copy link
Owner

I found one error so far: the applicationId was set to com.takisoft.preferencefix in the gradle file instead of com.takisoft.preferencex.demo. Changing it and cleaning the project helped 😅

@gregkorossy
Copy link
Owner

Well, on API 15 and 17 (so probably below API 21) the SeekBarPreference has bad padding.

API 15 API 17
screenshot_1531483967 screenshot_1531483976

@Mygod
Copy link
Contributor Author

Mygod commented Jul 13, 2018

Hmm yeah I didn't test API <19. 😅

@gregkorossy
Copy link
Owner

Ok, so I started investigating the layout files and it's "funny", the guys (once again) missed the background attribute in the base and v17 SeekBarPreference layouts which triggers the bug that was fixed by the custom PreferenceGroupAdapter earlier... Adding the simple android:background="?android:attr/selectableItemBackground" to the layouts' root fixes the padding problem, however, the misplaced SeekBar is still present on 17-19. The search continues...

@gregkorossy
Copy link
Owner

Well, found the other "easter egg":

This is how the SeekBar is aligned with the title:

android:paddingStart="@dimen/preference_seekbar_padding_start"
android:paddingEnd="@dimen/preference_seekbar_padding_end"

which just doesn't work, the paddings in the emulator are symmetrical instead of being 0dp on the left and 22dp on the right. How to fix it? Add the paddingLeft and paddingRight attributes too:

android:paddingStart="@dimen/preference_seekbar_padding_start"
android:paddingLeft="@dimen/preference_seekbar_padding_start"
android:paddingEnd="@dimen/preference_seekbar_padding_end"
android:paddingRight="@dimen/preference_seekbar_padding_end"

Now the SeekBar is aligned with the text above it. And where's the fun, you might ask. Well:

Redundant attribute paddingLeft; already defining paddingStart with targetSdkVersion 28.
Redundant attribute paddingRight; already defining paddingEnd with targetSdkVersion 28.

I mean, c'mon Google, get your shit together...

@gregkorossy
Copy link
Owner

@gregkorossy
Copy link
Owner

Also, committed a patch to the AOSP to fix this in AndroidX:
https://android-review.googlesource.com/c/platform/frameworks/support/+/717258

@Mygod
Copy link
Contributor Author

Mygod commented Jul 14, 2018

Huh nice. I'm guessing that not even Google cares about Android 4.x any more.

BTW how do you contribute to AOSP?

@gregkorossy
Copy link
Owner

gregkorossy commented Jul 14, 2018

Yeah, I get that Android 4.x is pretty old but if they officially support it by setting the minSdk to 14 then they should test everything at least once...

Check out the Submitting Patches guide for details about how to contribute to AOSP.

@Mygod
Copy link
Contributor Author

Mygod commented Jul 14, 2018

Thanks! Seems pretty complicated...

Redundant attribute paddingLeft; already defining paddingStart with targetSdkVersion 28.
Redundant attribute paddingRight; already defining paddingEnd with targetSdkVersion 28.

I think that's just a warning. You can suppress it using tools:ignore="RtlHardcoded". Is there anything else that's holding this release back? I just tested it on API 19 and it seems to work fine.

@gregkorossy
Copy link
Owner

I just submitted all of the artifacts to bintray and now waiting for it to be included in jcenter. If you want to use it right away, you need to add the repo URL https://dl.bintray.com/takisoft/android to your repos:

repositories {
    maven {
        url "https://dl.bintray.com/takisoft/android"
    }
}

@Mygod
Copy link
Contributor Author

Mygod commented Jul 15, 2018

Looks good! I'm already starting to use it in my own project. Kudos to you!

@gregkorossy
Copy link
Owner

gregkorossy commented Jul 15, 2018

Meanwhile they became available on jcenter, so you don't need to specify the repo URL separately.

@gregkorossy
Copy link
Owner

@Mygod Funny thing is, I reported the issue about the SeekBarPreference's bad paddings, the assigned guy asks for a sample project, but I just cannot reproduce it on API 17 in a new project. Tried to strip the sample project in this lib so that it would look like the one in the new project and it reproduces the issue... WTF? 😅 I just don't understand...

@gregkorossy
Copy link
Owner

Oh wow. So found the single attribute that decides whether it produces the error or not: android:supportsRtl in the manifest. If it's set to true, the paddings are good on API 17 (API 15 still looks bad) but if it's false (defaults to that), the paddingStart and paddingEnd attributes are not interpreted on pre-Lollipop devices.

@gregkorossy
Copy link
Owner

Deleted the dev-v28 branch and created a new one with the name androidx (so basically renamed it).

@Mygod
Copy link
Contributor Author

Mygod commented Jul 18, 2018

We should add the bugfix for SeekBarPreference to README as well.

@gregkorossy
Copy link
Owner

No need in my opinion as the lib contains the fixed layout files, and as soon as the patch gets reviewed and merged (and released ofc), I'll remove them.

@Mygod
Copy link
Contributor Author

Mygod commented Jul 18, 2018

Sounds good.

@feelfreelinux
Copy link

@Gericop @Mygod Hi, thanks a lot for this lib, helped me a lot in my projects. However, after updating to androidx, all of my settings fragment got additional margin from the left (As seen in this screenshot: https://user-images.githubusercontent.com/5181621/42691091-fdd97c6c-86a6-11e8-891f-a2460035e182.png)
Is there any way to disable it? Tried to @null icon / etc but it didn't work.

@gregkorossy
Copy link
Owner

That's because the new design reserves the icon space automatically so if you set an icon on one or more preferences, it wouldn't look awkward. That being said, you can turn it off by setting the app:iconSpaceReserved to false on every preference in the XML.

@feelfreelinux
Copy link

@Gericop Thanks, will test it out. The next thing i noticed, is that onSharedPreferenceChanged is not getting called at all. Its inflated by replacing FrameLayout with fragment from parent activity.

@feelfreelinux
Copy link

Btw, #132 still occures in alpha01

@gregkorossy
Copy link
Owner

@feelfreelinux What do you mean onSharedPreferenceChanged is not getting called? It works in the sample app.

@feelfreelinux
Copy link

@Gericop Oh, my fault. During AndroidX refactor, I accidentally removed onResume override where I was setting onSharedPreferenceChanged. Thanks a lot.

@adrcotfas
Copy link

@Gericop Hi there!
About that additional margin that @feelfreelinux mentioned, I see that it cannot be disabled like that for a PreferenceCategory too, just for a Preference. Is there any other way?

@gregkorossy
Copy link
Owner

gregkorossy commented Jul 19, 2018

It seems like the icon (and the space for it) is handled differently in the category layout than in a preference, and thus it does not work. I'll file a bug report with Google and provide a bug fix in this lib until it gets resolved officially.

@gregkorossy
Copy link
Owner

@adrcotfas @feelfreelinux Reopened the #132 issue. Also, published v1.0.0-alpha2 that contains the bug fix for this problem.

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.

4 participants