Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Fix issue 6878 | [Bug] ShellItem.Items.Clear() crashes when the ShellItem has bottom tabs #8215

Merged
merged 2 commits into from
Dec 2, 2019

Conversation

felipebaltazar
Copy link
Contributor

Description of Change

I followed another PR (#8079) feedback and create a null check to avoid NullException for ShellBottomNavViewAppearanceTracker.SetBackgroundColor

Issues Resolved

API Changes

Changed:

  • void ShellBottomNavViewAppearanceTracker.SetBackgroundColor
    • added this validation:
	var child = menuView.GetChildAt(index);

	if (child == null)
		return;

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

  1. Create a ShellItem in that contains tabs and set background-color
  2. In code, call Items.Clear() on your ShellItem.
  3. No exceptions at this point
  • Or just run 6878 issue at Control Galery:

image

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@samhouts samhouts added p/Android a/shell 🐚 hacktoberfest 🍻 i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often t/bug 🐛 labels Oct 25, 2019
@felipebaltazar
Copy link
Contributor Author

@PureWeen @rmarinho
Hello, any position about this PR?

@felipebaltazar
Copy link
Contributor Author

I`ve made the changes proposed by @PureWeen and updated this branch

@PureWeen
Copy link
Contributor

PureWeen commented Dec 1, 2019

@felipebaltazar can you rebase this to 4.4?

Then I'll give the UI tests one more run through and merge.

Thank you!!

@felipebaltazar
Copy link
Contributor Author

felipebaltazar commented Dec 1, 2019

@felipebaltazar can you rebase this to 4.4?

Then I'll give the UI tests one more run through and merge.

Thank you!!

@PureWeen I've rebased this branch to 4.4.0, but now we have 41 additional commits, seems like changes made on master.. so can I only revert theses commits?

@PureWeen
Copy link
Contributor

PureWeen commented Dec 1, 2019

@felipebaltazar you'll need to rebase and force push. If you're having issues I can give it a go

git rebase --onto 4.4.0 master issue-6878

Then fix merge conflicts

Then do a force push to your branch

@samhouts samhouts added i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often and removed ControlGallery Core a/Xaml </> a/carouselview a/collectionview a/visual i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often p/Android p/UWP p/WPF p/gtk p/iOS 🍎 p/macOS labels Dec 2, 2019
@felipebaltazar felipebaltazar deleted the issues-6878 branch December 2, 2019 23:39
@samhouts samhouts added this to the 4.4.0 milestone Dec 3, 2019
@minahenin
Copy link

Hi, This is still broken in IOS

even when the Tab has multiple ShellItems when I try to Remove the first item "Items.RemoveAt(0);" it still crashes.

@francis2
Copy link

francis2 commented Feb 17, 2020

Hi guys,

I am using Xamarin.Forms.4.5.0.282-pre4

I don't get crashes, but I am unable to remove the first item.

These calls do nothing. The first Item always remains in the Items collection.
Items.RemoveAt(0)
Items.Remove(Tab.CurrentItem)
Items.Clear()

In my use case, the top menus are created based on the user data. I must create a default placeholder in xaml, but i need to delete it (or make it invisible, but that is not available yet).

<TabBar>
    <Tab
        x:Name="mainTab"
        Title="SomeText"
        Route="someroute">
        <ShellContent
            Title="Empty"
            Route="blank">
            <ShellContent.ContentTemplate>
                <DataTemplate>
                    <MyPage>
                        <x:Arguments>
                            <x:String></x:String>
                        </x:Arguments>
                    </MyPage>
                </DataTemplate>
            </ShellContent.ContentTemplate>
        </ShellContent>            
    </Tab>
</TabBar>

EDIT
This may help: the Items collection is empty, but there is a VisibleItems collection that still contains the empty tab. I couldn't find a way to access it to clear it manually.

image

@PureWeen
Copy link
Contributor

@francis2 do you have a repro you can attach?

@francis2
Copy link

Sure, here you go.

PickerTestApp.zip

@PureWeen
Copy link
Contributor

Thank you @francis2

#9615

@francis2
Copy link

francis2 commented Mar 2, 2020

Hi guys,

Tab.Items.Clear() crashes on 4.5.0.356.

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index

Attached is a repro solution. Crash occurs in AppShell.xaml.cs line 28 on Tab.Items.Clear()

Thanks

PickerTestApp.zip

@PureWeen
Copy link
Contributor

PureWeen commented Mar 2, 2020

@francis2

Created this issue for tracking
#9801

I'll test and let you know over there what I find

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/shell 🐚 a/swipeview approved Has two approvals, no pending reviews, and no changes requested hacktoberfest 🍻 i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often t/bug 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants