AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
25.33k stars 2.2k forks source link

FilePicker keep showing when called from a Command in a SplitButton [repro provided] #12320

Open dbriard opened 1 year ago

dbriard commented 1 year ago

Describe the bug I have a SaveImage command binded to a SplitButton. In the attached repro, the SaveImage command just display a SaveFilePicker. When you click the SplitButton (or via SplitButton flyout menu), the FilePicker is showing, but if you modify the filename and click Ok, after the picker is closed, the SaveImage function is called again, and again, and again.... each time you change the filename in the FilePicker. Note: When use with a Button instead of a SplitButton, it work as expected (even if command is in a FlyoutMenu).

To Reproduce

  1. Load the attached Repro
  2. Click on the SplitButton, a SaveFilePicker is displayed with "Untitled.png" as default filename.
  3. Replace the filename with something like "lmkfdmlkm"
  4. Validate the SaveFilePicker
  5. You should see the SaveImage() method called again, and the SaveFilePicker display again...

AvaloniaSaveFilePickerBug.zip

dbriard commented 1 year ago

Any status about this?

timunie commented 1 year ago

@robloo do you have any idea about it?

robloo commented 1 year ago

I have no idea and would have to debug it when I have some time. The SplitButton is designed to work the same as Button and the command handling code was copied from Button too.

How this can be affected by a FilePicker is really strange as well.

robloo commented 1 year ago

I took a quick look at this. It isn't reproducible with Window 10 Pro. I tried with both the provided sample app and with a few tests in ControlCatalog.

I suspect it has something to do with the file save picker or Windows 11 rather than the SplitButton. Therefore, I don't plan to investigate further unless information points back to the SplitButton.

robloo commented 1 year ago

A few more thoughts:

  1. Try binding to just the method directly (avoiding MVVM toolkit usage)
  2. Try using the managed file picker instead of system-native one
  3. Try doing something other than opening a picker in the command

The answer to those checks should narrow this down quite a bit.

dbriard commented 1 year ago

@robloo Thank you to had a look.

This issue is very strange, I retry my repro sample on my Windows 10 machine after your comment yesterday and cannot reproduce it. At the same time I retry on Windows 11 and on 20 tries, I only had the issue 1 time. Finally, I retried again this morning on Windows 11 and I have the issue 100% of the time, as when I opened this issue...

I'll to copy SplitButton code in another class and see if I got the issue and why when I have time.

dbriard commented 1 year ago

@robloo An interesting find. It look like the issue only arise when I close the SaveFileDialog with Enter key (whatever I change the filename or not), no problem when I click on the Save button with mouse. And no problem with Button instead of SplitButton.

https://github.com/AvaloniaUI/Avalonia/assets/3595443/946f5598-e9a0-4597-ab5e-d46b75f23cf3

dbriard commented 1 year ago

@robloo I copy/pasted the code of SplitButton and I found the issue!

When I close the FilePicker by pressing enter, the SplitButton received the KeyUp event and call the command again... (I do not tested in Windows 10 yet).

image

Could be fixed by adding "_isKeyboardPressed &&" in the if I think. image

timunie commented 1 year ago

@dbriard looks reasonable to me to add this check. Maybe you can file a PR and see if any test fail and to check the PR in your App. If all positive, I guess it can be merged.

dbriard commented 1 year ago

@timunie I may give a try!

dbriard commented 1 year ago

@timunie I am trying to push my fix but I got the error below:

Failed to push to the remote repository. See the Output window for more details.
Pushing fix-12320
Remote: Permission to AvaloniaUI/Avalonia.git denied to dbriard.
Failed to push to the remote repository. See the Output window for more details.
Error encountered while pushing to the remote repository: Git failed with a fatal error.
Git failed with a fatal error.
unable to access 'https://github.com/AvaloniaUI/Avalonia/': The requested URL returned error: 403

I never contributed on Github, do you have to approve my account or something to contribute?

timunie commented 1 year ago

you need to fork the repo and then add a PR later from your fork / branch. I personally use GitKraken to handle all this.

Maybe this tutorial helps: https://www.freecodecamp.org/news/how-to-make-your-first-pull-request-on-github-3/

robloo commented 1 year ago

I want to understand this a but more before the fix is merged.

  1. Do we need to do somethung different with focus in this case. Technically the file picker should be in focus and we wouldn't receive ANY enter key again in SplitButton.
  2. Compare with other controls. Is this a problem with any other buttons?
  3. We also want to be moving away from invoking commands in key down. Release is how WPF and all other XAML frameworks work. This may be the difference with Button as well.
robloo commented 1 year ago

@maxkatz6 Since you implemented file pickers. Do you know of a simple way to pass focus to the picker itself while it's open? I know that is outside the visual tree but Avalonia must track this sort of thing elsewhere? If not, it might be something as simple a new field _externalFocusActive that can be checked on key press to filter this case out.

The more I think about this the more I realize the correct fix is probably to avoid the Enter press in a file picker ever being sent to the SplitButton.