Fix: Fixed issue where toolbar button sometimes had the wrong icon state

[jpozo20]

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

• Closes Bug: Delete icon appears disabled for image files #16715

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

1. Opened Files ...
2. Select an image file
3. Before the bugfix, some icons like Copy, Cut and Delete are shown as disabled even if they are enabled
4. With the bugfix all applicable icons show in color when selecting image files

[yaira2]

I can no longer reproduce the issue with these changes, LGTM.

@jpozo20 thank you for contributing to Files 🎉

[yaira2]

Hmm, it looks like it's still occurring.

[jpozo20]

@yaira2 how so? I tested image files, folders and some binaries files and enabled icons show correctly.

[yaira2]

It's hard to consistently reproduce hence the reason I thought it was resolved. I'm usually able to reproduce the issue by unfocusing and refocusing an image file a bunch of times.

[jpozo20]

I tried that and other things but I cannot longer reproduce it.
This is the code causing the issue. As per my comment in the commit, the issue was because _isOwnerEnabled sometimes was false even if the parent button has IsEnabled=true. Maybe a race condition where IsEnabled property from the ThemedIcon is not true?

else if (IsHighContrast is true || _isOwnerEnabled is false || IsEnabled is false)
{
	VisualStateManager.GoToState(this, OutlineTypeStateName, true);
	VisualStateManager.GoToState(this, DisabledStateName, true);
	return;
}

[yaira2]

One thing that's different about images, is that there are a couple of secondary actions added to the toolbar.

[yaira2]

@jpozo20 is it okay if I push an alternative fix to your branch for testing?

[jpozo20]

@yaira2 It's okay. Do I need to do something to allow the push?

[yaira2]

@jpozo20 I pushed my change in a separate PR, can you look it over and let me know if you can still reproduce the issue? #17032

[jpozo20]

@yaira2 I've been busy and did not have time to check but I see you have fixed it in the other PR

[Lamparter]

This can be closed now 🙂

[yaira2]

@jpozo20 thanks! Please let me know if you have any thoughts on my implementation.