FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(android-styling): correctly detect if drawable uses ColorFilter by edusperoni · Pull Request #6342 · NativeScript/NativeScript · GitHub

/ NativeScript Public

fix(android-styling): correctly detect if drawable uses ColorFilter - #6342

Merged
dtopuzov merged 20 commits into
NativeScript:masterfrom
edusperoni:fix-button-background
Nov 27, 2018
ConversationCommits (20)ChecksFiles changed
Merged

fix(android-styling): correctly detect if drawable uses ColorFilter#6342
dtopuzov merged 20 commits into
NativeScript:masterfrom
edusperoni:fix-button-background

Conversation

edusperoni commented Oct 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Checklist

I reran the tests on an emulator and they all succeeded. iOS tests output the same as without this change.

What is the current behavior?

Changing border-radius makes you unable to change color on Button if border-radius returns to 0

What is the new behavior?

When a background has border-radius and becomes a BorderDrawable, it is no longer considered a background which must have it's color changed by setColorFilter.

Fixes #6341

fix(android-styling): correctly detect if drawable uses ColorFilter
fdda551 
Treat a widget with a BorderDrawable background as any other, even if it's FilterOnly

If the drawable has been replaced with a new one, clear _cachedDrawable

Fixes NativeScript#6341
test(button): add test for changing border-radius and color in button
a506409
ghost added the ♥ community PR label Oct 3, 2018
ns-bot added the cla: yes label Oct 3, 2018

edusperoni commented Oct 3, 2018

Copy link
Copy Markdown
Contributor Author

Just ran the tests on iOS, they all passed except for a few that are unrelated to this change.

vakrilov commented Oct 8, 2018

Copy link
Copy Markdown
Contributor

Hey @edusperoni,
Thanks for the issue and the PR. We will review it gat back to you within a few days.
Indeed this PR should not cause any IOS tests to fail - so don't worry about that.

ghost assigned manoldonev Oct 12, 2018
ghost added in progress and removed ♥ community PR labels Oct 12, 2018

vakrilov commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

test

ghost assigned vakrilov Oct 22, 2018

manoldonev commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

test

vakrilov commented Nov 12, 2018

Copy link
Copy Markdown
Contributor

test

NativeScript deleted a comment from manoldonev Nov 12, 2018

vakrilov commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

test

vakrilov commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

Hey @edusperoni
Sorry for the delay.

All unit tests are green, however we picked a strange behavior that has changed in our e2e tests for android.

It can be reproduced by running this app (make sure you do npm run setup or link the tns-core-modules package in the app). Here are the steps:

  1. Run the app
  2. Type 3164 and navigate to the issue-3164 page
  3. Click on "set border to 3" button (it sets border color and width)
  4. Click on "set border to 0" button (it sets the border width back to 0)

We expect the button to regain its original drawable, but with this PR - it doesn't. I noticed that removing the line that clears the _cachedDrawable restores the original behavior. Unit tests seem to pass too. Can you please share whats the reason for adding that code?

edusperoni commented Nov 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The reason I added that line was that the moment we changed the background, the cacheddrawable is dirty. This also means the previous background is no longer being referenced, only it's constantState.

This line was added to make sure we're not caching old drawables when creating new ones. So once the view gets a new drawable, it no longer returns to an old drawable.

Now I see it may cause this issue on the specific cases (like button) when setting background to empty means returning to the original (not null/BorderDrawable).

We also had experienced some issues in our projects where if we changed the background to a color and back (something like class="selected" sets a background color, and when removed there is no color), it would try to use the previous state that had been GCed by that point (NativeScript/android#887 on this specific line

NativeScript/tns-core-modules/ui/styling/background.android.ts

Line 78 in 9719a53

defaultDrawable = cachedDrawable.newDrawable(nativeView.getResources())
), so even referencing this native object in a member variable isn't enough to make the GC not collect it.

I believe that line may be removed, but there's probably some underlying issue in caching this StateDrawable,

radeva commented Nov 19, 2018

Copy link
Copy Markdown

Hi @edusperoni ,

Congratulations on being one of the winners in the {N} First-time contributors contest!

You can claim your prize by contacting us at nativescriptwinners[at]progress.com not later than Nov 30th 2018 . Make sure you send us the following info: 
👉Your full name 
👉Your email 
👉Your country or residence 

Best regards,
The NativeScript Team

vakrilov commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

Hey @edusperoni,
I would suggest removing the line that clears the cache as it breaks the test.

Do you think you can reproduce any of the other issues you mentioned? It looks to me that they are not related to this PR. Maybe we can work on them separately as long as this PR does not cause this bug to appear more often.

vakrilov commented Nov 23, 2018

Copy link
Copy Markdown
Contributor

test

ghost assigned SvetoslavTsenov Nov 23, 2018

SvetoslavTsenov commented Nov 23, 2018

Copy link
Copy Markdown
Contributor

test

ghost assigned dtopuzov Nov 26, 2018

dtopuzov commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

test

dtopuzov commented Nov 27, 2018

Copy link
Copy Markdown
Contributor

test

dtopuzov merged commit 11d3884 into NativeScript:master Nov 27, 2018
ghost removed the in progress label Nov 27, 2018

lock Bot commented Nov 27, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock Bot locked and limited conversation to collaborators Nov 27, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
manoldonev manoldonev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

SvetoslavTsenov

vakrilov

dtopuzov

Labels

cla: yes ♥ community PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[android] Can't reset button border-radius

7 participants

Back | FazBrowse Home | New Git URL