Skip to content
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

Should a click on a MetroTabItems close button really clear the buttons Command and CommandParameter properties? #2462

Closed
davikor opened this issue Apr 7, 2016 · 6 comments
Assignees
Milestone

Comments

@davikor
Copy link

davikor commented Apr 7, 2016

What steps will reproduce this issue?

Use MetroTabControl and add MetroTabItems with CloseButtonEnabledset to true and CloseTabCommand (of the MetroTabItem) bound to a command that triggers a dialog ("are you sure you want to close...blablabla?"). When denied, the command does not remove the tab, but closeButton_Click in MetroTabItem.cs sets the CloseTabCommand and CloseTabCommandParameter properties to null, preventing subsequent command invocations.

Expected outcome

In my opinion these two properties should remain set to their respective Command and Parameter or am I using this control not in the intended way?

Environment

  • MahApps.Metro 1.2.4 as well as on develop and master branches
  • Windows 7
  • Visual Studio 2015
  • .NET Framework 4.6
@punker76
Copy link
Member

punker76 commented Apr 7, 2016

@lagwagon667 Just looked at this and think that we maybe doesn't need that. The fix is that we should check the CanExecute for false too and don't execute the code behind that.

@punker76 punker76 added this to the 1.3.0 milestone Apr 7, 2016
@punker76 punker76 self-assigned this Apr 7, 2016
@davikor
Copy link
Author

davikor commented Apr 7, 2016

I don't think checking CanExecute will be sufficient for above use case. If you want to ask the user whether she really wants to close the tab, you can only do this in the Execute method. Why do you remove the references to the command and its parameter anyway?

@punker76
Copy link
Member

punker76 commented Apr 7, 2016

@lagwagon667 I don't know, it's old code, must look at this...

punker76 added a commit that referenced this issue Apr 14, 2016
@punker76
Copy link
Member

@lagwagon667 I fix clearing the commands (I understood now why we does it). But a note, the commands on the MetroTabItem itself are not a good place for doing this what you want. It's better you use the TabItemClosingEvent on the MetroTabControl. You can set the Cancel to true if you don't want closing the MetroTabItem.

Hope this helps!

@punker76
Copy link
Member

@lagwagon667 I put a refactoring note on my todo list for v2.0, cause I don't like what we do with this command closing stuff.

@bboxstart
Copy link

bboxstart commented Mar 21, 2017

@punker76 I just encountered this behaviour this week. Using the event is no solution when applying MVVM. However, I was able to work-around this by using the CloseTabCommand on MetroTabControl instead of the same parameter on MetroTabItem. You then still can use the CloseTabCommandParameter on the MetroTabItem.

However, I do support @lagwagon667 on:

  • Using CanExecuteCommand is not a solution when you would like the user to ask for confirmation
  • The behaviour as is when using CloseTabCommand of MetroTabItem is not intuïtive.

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

No branches or pull requests

3 participants