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

Don't render empty TextCells for TableSections without Title (bugs 26104 and 42926) #287

Merged
merged 2 commits into from Dec 6, 2016

Conversation

Projects
None yet
5 participants
@MichaelRumpler
Contributor

MichaelRumpler commented Aug 9, 2016

Description of Change

When a TableSection has no header, then iOS doesn't render a header cell. Android rendered an empty TextCell and a separator line in the accent color. This could not be removed by the user.

With this change, the empty TextCell and the separator are not rendered. If you still want them, then you can set the Title to " ".

When I debugged the GetCellForPosition method, I saw that it was called very often. Before a TableView was rendered, the method was called twice for each Cell from GetView and twice for each Cell from IsEnabled. I removed one call from GetView, but IsEnabled is called twice from the adapter. I think the result of GetCellForPosition should be cached in a Cell[] or WeakReference[] and two bool[] for isHeader and nextIsHeader. Then it would not have to iterate over all Cells every time the method is called.
But for this PR I only wanted to fix the bugs and didn't want to mix it up with caching.

Bugs Fixed

API Changes

None

Behavioral Changes

None

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
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Aug 9, 2016

Hi @MichaelRumpler, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Aug 9, 2016

Hi @MichaelRumpler, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Aug 9, 2016

Member

Can you rebase with master please ?

Member

rmarinho commented Aug 9, 2016

Can you rebase with master please ?

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Aug 9, 2016

Contributor

I committed my change locally, then I did a
git rebase -i HEAD~1
which displayed only my one commit with "pick" and then I pushed.

What did I do wrong?

Contributor

MichaelRumpler commented Aug 9, 2016

I committed my change locally, then I did a
git rebase -i HEAD~1
which displayed only my one commit with "pick" and then I pushed.

What did I do wrong?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Aug 9, 2016

Member

Is your branch master synced with ours? because i was to run uitest and it failed because seems you are missing a fix we did on master.

Member

rmarinho commented Aug 9, 2016

Is your branch master synced with ours? because i was to run uitest and it failed because seems you are missing a fix we did on master.

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Aug 9, 2016

Contributor

I forked it just minutes before I made the PR because when I tried to do the PR from my old fork, it also listed an old commit which has already been merged. So I deleted my repository and created it new.

Sorry, I don't know how rebase works and it seems like I always do it wrong.

Contributor

MichaelRumpler commented Aug 9, 2016

I forked it just minutes before I made the PR because when I tried to do the PR from my old fork, it also listed an old commit which has already been merged. So I deleted my repository and created it new.

Sorry, I don't know how rebase works and it seems like I always do it wrong.

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Aug 9, 2016

Contributor

I found another page which explained rebase and did:

git remote add xamarin https://github.com/xamarin/Xamarin.Forms.git
git fetch xamarin
git rebase -i HEAD~1
git rebase xamarin/master
git push -f

It doesn't seem like this PR changed, but I don't know what else I can do.

PS: What I forgot in the description, but it's obvious anyway: I did not write any tests because my change is only in Platform.Android and there are no tests for that project. I would've preferred to write tests for that too.

Contributor

MichaelRumpler commented Aug 9, 2016

I found another page which explained rebase and did:

git remote add xamarin https://github.com/xamarin/Xamarin.Forms.git
git fetch xamarin
git rebase -i HEAD~1
git rebase xamarin/master
git push -f

It doesn't seem like this PR changed, but I don't know what else I can do.

PS: What I forgot in the description, but it's obvious anyway: I did not write any tests because my change is only in Platform.Android and there are no tests for that project. I would've preferred to write tests for that too.

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Aug 11, 2016

Contributor

After consulting with Rui I also committed the caching of the Cells like I explained in the description.
Now GetCellForPosition doesn't have to iterate over all Cells every time it is called. It iterates over all Cells in the TableView (a TableView shouldn't have thousands of items as ListView might) once and then uses this cache until Controller.ModelChanged.
The cache is also emptied in Dispose, but that was not called, so I added a call in TableViewRenderer.Dispose.

(I hope I got rebase right at last)

Contributor

MichaelRumpler commented Aug 11, 2016

After consulting with Rui I also committed the caching of the Cells like I explained in the description.
Now GetCellForPosition doesn't have to iterate over all Cells every time it is called. It iterates over all Cells in the TableView (a TableView shouldn't have thousands of items as ListView might) once and then uses this cache until Controller.ModelChanged.
The cache is also emptied in Dispose, but that was not called, so I added a call in TableViewRenderer.Dispose.

(I hope I got rebase right at last)

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Aug 11, 2016

Contributor

Another thing which came into mind: the whole code is not thread save. But most of the code in XF is not and I expect code from the Renderers to be called only from the UI thread anyway, so I don't think this is a problem.

Contributor

MichaelRumpler commented Aug 11, 2016

Another thing which came into mind: the whole code is not thread save. But most of the code in XF is not and I expect code from the Renderers to be called only from the UI thread anyway, so I don't think this is a problem.

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Aug 12, 2016

Contributor

I renamed the fields to start with an underscore and added private properties (pascal case) which call FillCache().

Contributor

MichaelRumpler commented Aug 12, 2016

I renamed the fields to start with an underscore and added private properties (pascal case) which call FillCache().

@jassmith

This comment has been minimized.

Show comment
Hide comment
@jassmith

jassmith Aug 30, 2016

Member

This PR is under testing. So far it looks pretty good I just need to review the memory implications.

Member

jassmith commented Aug 30, 2016

This PR is under testing. So far it looks pretty good I just need to review the memory implications.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Sep 27, 2016

Member

Hi @MichaelRumpler sorry can you rebase again, so i can run tests on iOS10 , thanks!

Member

rmarinho commented Sep 27, 2016

Hi @MichaelRumpler sorry can you rebase again, so i can run tests on iOS10 , thanks!

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Sep 29, 2016

Contributor

I'd be very surprised if my changes to Android files only would be influenced by iOS10, but I did so.

Contributor

MichaelRumpler commented Sep 29, 2016

I'd be very surprised if my changes to Android files only would be influenced by iOS10, but I did so.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Sep 30, 2016

Member

Something went wrong with rebase.

Member

rmarinho commented Sep 30, 2016

Something went wrong with rebase.

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Oct 4, 2016

Contributor

Should be fixed now.

Contributor

MichaelRumpler commented Oct 4, 2016

Should be fixed now.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Oct 4, 2016

Member

Hey @MichaelRumpler How does it render on UWP? A header with no text

Member

rmarinho commented Oct 4, 2016

Hey @MichaelRumpler How does it render on UWP? A header with no text

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Oct 4, 2016

Contributor

I changed two files in Android only. The two bugs I linked are about Android only. Those two Android bugs are fixed.

If there are other bugs for UWP, then you have to fix it yourself. >:(

Contributor

MichaelRumpler commented Oct 4, 2016

I changed two files in Android only. The two bugs I linked are about Android only. Those two Android bugs are fixed.

If there are other bugs for UWP, then you have to fix it yourself. >:(

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Oct 4, 2016

Member

The question wasn't if there was any bugs, rather that if the behaviour is now consistent between platforms.

Thanks, i will check.

Member

rmarinho commented Oct 4, 2016

The question wasn't if there was any bugs, rather that if the behaviour is now consistent between platforms.

Thanks, i will check.

@MichaelRumpler

This comment has been minimized.

Show comment
Hide comment
@MichaelRumpler

MichaelRumpler Nov 29, 2016

Contributor

removed linq

Contributor

MichaelRumpler commented Nov 29, 2016

removed linq

@rmarinho rmarinho merged commit 75258aa into xamarin:master Dec 6, 2016

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

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