dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.04k stars 1.17k forks source link

Remove underscore from WPF event method names #403

Open xsoheilalizadeh opened 5 years ago

xsoheilalizadeh commented 5 years ago

_This issue has forwarded from Twitter._ I've created a new .NET Core base WPF with CLI and have got Hello world sample with the following command:

dotnet new wpf

After that, I've seen the event method for exit from the app, that its name was ButtonExit_Click. This issue is for the name convention of WPF event I think it would be nice when we remove the underscore ( _ ) from the method name. This isn't interesting for clean code and we could improve that.

private void ButtonExit_Click(object sender, RoutedEventArgs e){}

private void ButtonExitClick(object sender, RoutedEventArgs e){}

the sample repo

/cc @karelz

karelz commented 5 years ago

This is designer imperfection. @fabiant3 @diverdan92 is this the right place to track such issues / improvements?

miloush commented 5 years ago

This only happens when the designer autogenerates the event handler. The developer can always provide any name they like and then hit F12 to have it generated.

I don't like the one without underscore for example. Not only it's not following the guidelines either (which suggest methods should begin with a verb), but it is misleading as well, as it does not cause the button to be clicked. If the generated name should be updated, I think OnButtonExitClick~ed~ would be a better option, because it's invoked after the action and indicates it's an event handler.

Also note that the same name is used when you use Tab keys to generate the event handler in code, i.e.

ButtonExit.Click += ButtonExit_Click;

I would expect it unlikely to get these (i.e. code and designer generated) methods generate different names.

bigworld12 commented 5 years ago

the problem here is the naming convention itself, it would be better to have Clicked event when the button is clicked, rather than a Click event which isn't very intuitive

bigworld12 commented 5 years ago

but I don't think such issues are really worth it, since it's a breaking change and it's just a naming convention

miloush commented 5 years ago

@bigworld12 The problem with Clicked is that you need to inflect the verb, which not only is hard by it self, but is neither always possible nor desirable. There are events like Closed and Closing, GiveFeedback and GotFocus, KeyDown or ManipulationBoundaryFeedback. Changing _ into On prefix is the only idea I have that would work for all of them and is consistent with virtual methods (there are already OnClosed and OnClosing overrides for example).

I do agree though that such change might not be worth the effort.

stevenbrix commented 5 years ago

I'm keeping this open but moving to future. There is some good discussion here, although i think a few things have come up:

  1. Click is a bad name for an event. Should be Clicked.
  2. Renaming default method name from ButtonExit_Click to OnButtonExitClick (as an example)
dotMorten commented 5 years ago

I think OnButtonExitClick would be a better option

I vote no to the On prefix. That's typically used for virtual event methods for override on the instance that defines that event (as opposed to here reacting to it)

miloush commented 5 years ago

@dotMorten You can take this as an argument for yes too. The virtual methods have usually different signature so there is no conflict and they already mean the same thing (i.e. they are called when the event is occurring).

I actually use On prefix based on what the handler does, e.g. if it just logs things, I would use On, if it should e.g. sort things on click, I would probably just call it SortThings. But it doesn't matter, obviously different people prefer different styles and we won't be able to come to a solution that everybody is happy with.

@karelz the issue does not mention designer, it is about the code in a shipping sample. Perhaps that can be fixed easily without changes in the designer.

I originally raised a technical point which is that the generated name is the same that code generates when you use the Tab key. WPF designer might even use the same code path, who knows. If that parity is to be preserved, this is a wrong repo to suggest the change. Don't forget that WinForms also generates event handlers the same way, so arguments about ".NET Core base" need to target all of WPF designer, WinForms designer, WinForms property pane, C# editor and likely VB.NET editor too.

@stevenbrix Let's step back and consider whether anything can change here. You cannot rename Click because it breaks a lot of existing code. You can depart from the code generated name elsewhere for new code, just to make a different half of users unhappy. Current options for unhappy users are:

  1. type the handler name yourself and press F12, it will get generated in codebehind as typed
  2. use the default name when generating it, and F2 to rename it

These are pretty easy and straightforward workflows. Code bases will always be polluted by people who don't care and/or by code where care is unnecessary, I don't think that should be an argument.

You can also introduce a code style option, that seems to be popular nowadays. To me, any proper change here appears to me as a lot of work with no real benefit - there are more important problems and tooling improvements to spend it on. That said, I belong to the unhappy group for both current and proposed naming, so I am not against anyone contributing changes here.

fabiant3-zz commented 5 years ago

@maxbrister for designer improvment awareness