#1668 tab container panic when removing tabs

[adrianre12]

Description:

As requested by Andrew Williams this is based on the v1.4.2 branch
To fix the panic after tab deletion when the selected tab is out of range of the TabContainer.items slice. I have changed the code so that TabContaner.current value is updated.
If the selected tab index is higher than that of the deleted tab, the current value is decreased by one. E.G. tabs A B C with C selected, current=2. Delete B, tabs become A C and current=1 so that it still points to C.
If the selected tab index is equal to that of the deleted tab. The value of current is set to -1 which represents no selection
E.G. tabs A B C with B selected current=1. Delete B and current=-1.

There was also an issue where the underline was remaining if the last tab was deleted. To fix this TabContainer.movSelection now hides the underline when current=-1

Finally an unrelated issue, .gitignore is not ignoring windows executables. I saw that it has been setup to ignore linux executables so have added the .exe files too.

As far as I can tell all the TabContainer tests pass, but I can't get TestEntry_Select to pass but it looks like that fails for me on the v1.4.2 branch too.

Fixes #1668

[Jacalz]

I think there was a misunderstanding. The PR should be based on and have the release/1.4.x branch as the target to merge into. Everything will be added to master when the release happens.

[adrianre12]

It is my first time using github PR. I am not sure if I understand.
I forked fyne-io and checked out release/1.4.x branch, did my changes and created the PR
The reason I mentioned v1.4.2 is in the PR template is says to use branch develop. Did I do it wrong?

[Jacalz]

Ah, then I really must congratulate you. Congratulations for both creating your first PR and your first contribution to Fyne 🎉

The target branch is where you want your PR to end up when merged. 9/10 times this will be develop, but when doing small bug fixes that are intended for a bug fix release, it should target the latest release branch (release/1.4.x in this case). You have based the PR on that branch, so you are off to a really good start. Now we just need to make it target that branch too 🙂

You can change the target branch of the PR (apparent GitHub calls this “base branch”) by pressing edit where you usually edit the title of the PR and then change the little select button to say that you want to merge it into the name of the branch (release/1.4.x in this case). I did this for you now, but it looks like GitHub didn’t change much. Strange.

[andydotxyz]

I think all the code / git work was correct, when creating a PR it defaults to master - if you'd picked release/v1.4.x when opening the PR it would have been flawless ;)

[andydotxyz]

Thanks so much for your contribution.

Can you please add a test indicating what breakage has been fixed?

[stuartmscott]

Nice work @adrianre12, thanks for the fix! I added a few comments, but otherwise looks good

[adrianre12]

I have been looking at the tests and saw I had missed that TabContainer_DynamicTabs is now failing. I now realise I have made changes to the TagContainer by altering its behaviour when a tab is selected after tab removal. To me with my old QA manager hat on non backwardly compatible changes in a point release would be unacceptable, but it all depends on what your views are.
I am quite willing to undo the breaking changes and limit this to just fixing the panic, This should allow the TabContainer_DynamicTabs test to pass. Then raise another PR based on whatever branch you wish with the other changes if you still feel they are applicable. In summary, they are;

• If selected_index = removed_Index deselect tab (current=-1) . Currently, the selected_index does not change which means the selected tab effectively shifts to the next tab. If this now points to an out of range Item we get a Panic. The fix for the panic should be if selected_index >= len(items) deselect tab.
• If selected_index > removed_Index decrease selected_index by one so that it remains pointing to the same tab. Currently, the selected_index does not change which means the selected tab effectively shifts to the next tab.

[stuartmscott]

You're absolutely right, we try to avoid non-backwardly compatible changes in point releases. Minimizing this PR to just the panic and moving the other behaviour to another PR (on develop branch) feels like the right approach.

I think it would suffice for the change to be;

if c.current >= removed {
    c.current--
}

This way closing a tab after the current tab has no effect, closing a tab after the current tab shift the index down so the same tab is still selected, and closing the current tab selects the previous tab, if any, else selects -1

[adrianre12]

Scott, that would still be a breaking change. Currently, if we have tabs A B with A selected, remove A and B is selected. With that change, no tab would be selected. I am pretty sure this would make TabContainer_DynamicTabs fail.
I tried this yesterday and it worked.
if current >= len(Items) { current = -1 }
This does not change behaviour as at the moment it would panic.

[adrianre12]

I have removed all the feature changes so only the panic fix remains and added a test for the panic, should be good to go now

[stuartmscott]

Thanks for this @adrianre12

[andydotxyz]

Looks great, but I think it's missing a test for the case when len(c.Items) is > 0.
i.e. if it goes from 3 to 2 items (just to differentiate from the -1 case tested)

[andydotxyz]

Sweet, thanks

[andydotxyz]

Pending change requests expired. Approved.