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

Bug 51536, initial iOS cell layout is incorrect when cell has context actions … #787

Merged
merged 7 commits into from Mar 14, 2017

Conversation

Projects
None yet
7 participants
@VitalyKnyazev
Contributor

VitalyKnyazev commented Feb 26, 2017

Description of Change

Bug 51536, initial cell content is not aligned properly when cell has context actions or ListView is in recycle mode, but content gets aligned properly after scrolling out and back. See attached screenshots and sample project, add it to Xamarin.Forms solution.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=51536

API Changes

None

Behavioral Changes

Not sure why this check was there in the first place, could not find any side effects.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

before
after
App1.zip

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Feb 26, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

dnfclas commented Feb 26, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@StephaneDelcroix StephaneDelcroix requested review from hartez and removed request for hartez Feb 27, 2017

@VitalyKnyazev VitalyKnyazev changed the title from Bug 51536, initial height is incorrect when cell has context actions … to Bug 44525, initial height is incorrect when cell has context actions … Feb 28, 2017

@VitalyKnyazev VitalyKnyazev changed the title from Bug 44525, initial height is incorrect when cell has context actions … to Bug 44525, initial iOS cell height is incorrect when cell has context actions … Feb 28, 2017

@@ -87,7 +87,7 @@ public override void LayoutSubviews()
{
base.LayoutSubviews();
if (_scroller == null || (_scroller != null && _scroller.Frame.Width == ContentView.Bounds.Width))

This comment has been minimized.

@VitalyKnyazev

VitalyKnyazev Mar 2, 2017

Contributor

When "_scroller.Frame.Width == ContentView.Bounds.Width" is true then no further layout is happening and cell view is broken

@VitalyKnyazev

VitalyKnyazev Mar 2, 2017

Contributor

When "_scroller.Frame.Width == ContentView.Bounds.Width" is true then no further layout is happening and cell view is broken

@VitalyKnyazev VitalyKnyazev changed the title from Bug 44525, initial iOS cell height is incorrect when cell has context actions … to Bug 44525, initial iOS cell layout is incorrect when cell has context actions … Mar 6, 2017

@samhouts samhouts self-assigned this Mar 8, 2017

@rmarinho

this is a performance hit.. we shouldn't need to relayout if we are the same size, (width and height) Maybe the fix is check if the height is also the same ?

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Mar 9, 2017

Contributor

There is another bug regarding Cell layout and ContextActions on iOS: https://bugzilla.xamarin.com/show_bug.cgi?id=33450
Will this be fixed with this PR too?

Contributor

MichaelRumpler commented Mar 9, 2017

There is another bug regarding Cell layout and ContextActions on iOS: https://bugzilla.xamarin.com/show_bug.cgi?id=33450
Will this be fixed with this PR too?

@VitalyKnyazev

This comment has been minimized.

Show comment
Hide comment
@VitalyKnyazev

VitalyKnyazev Mar 9, 2017

Contributor

@rmarinho Maybe the fix is check if the height is also the same?

Thanks, check for both width and height works too, updated PR.

Contributor

VitalyKnyazev commented Mar 9, 2017

@rmarinho Maybe the fix is check if the height is also the same?

Thanks, check for both width and height works too, updated PR.

@rmarinho

The fix looks good. Just a couple of fixes needed:
Can you fix indentation and add the test case to the issues project.
Thanks
Thanks

@VitalyKnyazev

This comment has been minimized.

Show comment
Hide comment
@VitalyKnyazev

VitalyKnyazev Mar 9, 2017

Contributor

@rmarinho Indentation fixed, added test case but not sure it is as expected.

Contributor

VitalyKnyazev commented Mar 9, 2017

@rmarinho Indentation fixed, added test case but not sure it is as expected.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 9, 2017

Member

@VitalyKnyazev yap that's it, but seems that test file doesn't have the propper formating. We use tabs. You can read on the Readme about coding style .

Member

rmarinho commented Mar 9, 2017

@VitalyKnyazev yap that's it, but seems that test file doesn't have the propper formating. We use tabs. You can read on the Readme about coding style .

@VitalyKnyazev

This comment has been minimized.

Show comment
Hide comment
@VitalyKnyazev

VitalyKnyazev Mar 9, 2017

Contributor

@rmarinho Fixed, some lines were copy/pasted.

Contributor

VitalyKnyazev commented Mar 9, 2017

@rmarinho Fixed, some lines were copy/pasted.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 10, 2017

Member

There's 1 test failing

Bugzilla31330 can you take a look?

Thanks

Member

rmarinho commented Mar 10, 2017

There's 1 test failing

Bugzilla31330 can you take a look?

Thanks

@samhouts

I'm afraid this is not a fix for 44525. 44525's main issue is that the size does not increase when ForceUpdateSize is called. HOWEVER, I believe you have fixed one of the issues that has prevented me from pushing the fix for 44525 thus far. Thank you for that! I am requesting that you rename your reproduction to the 51536 bug instead of 44525, as the proper 44525 repro is now included in #809.

@VitalyKnyazev VitalyKnyazev changed the title from Bug 44525, initial iOS cell layout is incorrect when cell has context actions … to Bug 51536, initial iOS cell layout is incorrect when cell has context actions … Mar 11, 2017

@VitalyKnyazev

This comment has been minimized.

Show comment
Hide comment
@VitalyKnyazev

VitalyKnyazev Mar 12, 2017

Contributor

@samhouts done, please review

Contributor

VitalyKnyazev commented Mar 12, 2017

@samhouts done, please review

@gentledepp

This comment has been minimized.

Show comment
Hide comment
@gentledepp

gentledepp Mar 12, 2017

I just found out that this actually is our issue!
It would be so great to have it fixed!!! Thanks @VitalyKnyazev

gentledepp commented Mar 12, 2017

I just found out that this actually is our issue!
It would be so great to have it fixed!!! Thanks @VitalyKnyazev

@gentledepp

This comment has been minimized.

Show comment
Hide comment
@gentledepp

gentledepp Mar 14, 2017

@samhouts
VitalyKnyazev renamed the bug. Could you now please confirm? We are eagerly waiting for that fix. :-|

gentledepp commented Mar 14, 2017

@samhouts
VitalyKnyazev renamed the bug. Could you now please confirm? We are eagerly waiting for that fix. :-|

@samhouts samhouts merged commit cf21389 into xamarin:master Mar 14, 2017

7 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3744, ignored: 10
Details
Windows-Manual-Debug Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Manual PR Review : Tests passed: 3744, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

rmarinho added a commit that referenced this pull request Mar 14, 2017

Bug 51536, initial iOS cell layout is incorrect when cell has context…
… actions … (#787)

* Bug 51536, initial height is incorrect when cell has context actions or ListView is in recycle mode

* Improved fix for iOS uneven height cell layout

* Fixed indentation and added test case for issue 44525

* Fixed indentation for test case

* Added 1px room, this will fix Bugzilla31330 test, renamed test case to 51536

* Updated shared project file
@gentledepp

This comment has been minimized.

Show comment
Hide comment
@gentledepp

gentledepp Mar 23, 2017

His lead to the following regression bug: https://bugzilla.xamarin.com/show_bug.cgi?id=53834

Don't you guys have Xamarin.UITests for xamarin.forms?
I mean you are writing and talking about "quality improvements" all the time nowerdays - I'm just wondering how you are supposed to achieve that without tests? :-|

gentledepp commented Mar 23, 2017

His lead to the following regression bug: https://bugzilla.xamarin.com/show_bug.cgi?id=53834

Don't you guys have Xamarin.UITests for xamarin.forms?
I mean you are writing and talking about "quality improvements" all the time nowerdays - I'm just wondering how you are supposed to achieve that without tests? :-|

@samhouts samhouts added D15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.4 Jun 27, 2018

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