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

Button.Clicked is Action and not an event? #101

Closed
MihaMarkic opened this issue Jun 7, 2018 · 17 comments
Closed

Button.Clicked is Action and not an event? #101

MihaMarkic opened this issue Jun 7, 2018 · 17 comments
Labels
design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...)

Comments

@MihaMarkic
Copy link
Contributor

Why is Button.Clicked (and I assume other events) implemented as Action but not as an event?
I'd say event is more natural and standard.

@pmsanford
Copy link
Contributor

Same is true of RadioGroup.SelectionChanged but not TextField.Changed

@MihaMarkic
Copy link
Contributor Author

Should I create a PR to fix this by converting Action to EventHandler?

@migueldeicaza
Copy link
Collaborator

I have just never appreciated EventHandler types. They were invented in an era that lacked lambdas. My problem with it is two fold: the first parameter is rarely used and when you need it lambdas are now available. The second is that the payload to the event rather than being just a value is another type, this is just too much boilerplate in my view.

Am I missing something?

@pmsanford
Copy link
Contributor

The benefits of events as I see it are:

  1. The familiarity of the pattern to people who use the .NET Framework libraries that implement events
  2. The more obvious/less cumbersome way to attach and detatch multiple handlers
  3. The ability to control how you store multiple handlers (by overriding the event's add and remove handlers)
  4. The built in pattern of communicating things back to the caller or between different handlers

I'd say the latter three are pretty niche cases, though, at least in the context of a UI library. Personally, I don't have strong feelings either way, other than a bias for consistentcy

@ericsink
Copy link

ericsink commented Jun 9, 2018

I share Miguel's scorn for events.

That said, consistency is a good thing. Sometimes the right answer is to make things consistently awful. If so, events are the perfect way to achieve that.

@biohazard999
Copy link

I feel they are as noise.

var btn = new Button();
btn.Click += () => Console.WriteLine("Clicked");

//VS
var btn = new Button();
btn.Click += (sender,args) => Console.WriteLine("Clicked");

//Long
var btn = new Button();
btn.Click += Clicked;

void Clicked() => Console.WriteLine("Clicked");

//VS
var btn = new Button();
btn.Click += Clicked;

void Clicked(object sender, EventArgs args) => Console.WriteLine("Clicked");

@pmsanford
Copy link
Contributor

Sorry, I meant consistency within the library (TextField.Changed is event vs RadioGroup.SelectionChanged is Action). I agree that events don't bring much to the table here.

@biohazard999
Copy link

So if for consistency, then pls go for action, there is almost no need for the sender args pattern. :)

@MihaMarkic
Copy link
Contributor Author

There are other reasons for events instead of actions besides ones mentioned by @pmsanford.
On top of my head:
Such as converting events to Tasks or using them with Reactive stuff - I think there are utilities that handle these easily with events. Not sure about actions, but it shouldn't be hard anyway.
Also, what if I want to handle multiple buttons with a single method? I'd cast sender to button and then do whatever I'd do.

@pmsanford
Copy link
Contributor

You can do that yourself without the as/null check or is with a closure like

void HandleClicked(Button button) => Console.WriteLine($"Clicked {button.Id}");

...

var btn = new Button() { Id = "HappyButton" };
btn.Click = () => HandleClicked(btn);

@MihaMarkic
Copy link
Contributor Author

Sure, it's doable. But is it optimal? Compiler creates a hidden class just for that.

@migueldeicaza
Copy link
Collaborator

Good point folks - I am on vacation this week, with limited access to the internet, but will join this conversation next week. There seem to be two issues, one of consistency (public delegate vs event) and whether the signature should be EventHandler or the newer Action/Func pattern.

Will ponder here next week.

@Slesa
Copy link
Contributor

Slesa commented May 13, 2019

Introducing events would be a lie. We do not have concurrent tasks, no UI thread in the background, no devices sending something. When it comes to the terminal, we have an endless loop, polling the keyboard and the mouse - which means essentially waiting for the user - and meanwhile doing things.

Regarding the interaction between menu and a statusbar, I'd really appreciate to use new StatusBarMessage(menuItem.HelpText) to send sth to the bar. But it doesn't feel right here.

Not sure if that somehow interferes with the reactive stuff...

@bryancostanich
Copy link

FWIW, I vote for events. They're standard, a lot of libraries and things are built on top of them. And I'm not sure it's possible to remove an action. Whereas with events I can remove a handler via -=.

@migueldeicaza
Copy link
Collaborator

You can remove an action by clearing it: control.Action = null

But I hear you, you want events, I need to digest this, I think I will make the change, but it will break people

@tig
Copy link
Collaborator

tig commented May 24, 2020

Sorry I didn't see this Issue when I created this one: #447

I'm clearly in the EventHandler camp. For all the reasons above, plus: Action doesn't support multiple subscribers. EventHandler is designed to be used in a pub/sub way.

I think Action is awesome when used for internal-details. But for public APIs (and Terminal.Gui is a public API) EventHandler should be the norm.

@tig tig added the design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...) label May 24, 2020
@tig
Copy link
Collaborator

tig commented Jun 4, 2020

Dupe of #447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...)
Projects
None yet
Development

No branches or pull requests

8 participants