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

Added interface to set radio default option #229

Merged
merged 2 commits into from
Apr 17, 2019

Conversation

Girbons
Copy link
Contributor

@Girbons Girbons commented Apr 16, 2019

No description provided.

@okratitan
Copy link
Member

okratitan commented Apr 16, 2019

Pasting our chat log from the slack channel discussing the changes necessary:

Stephen Houston [2:02 PM]
@alessandro De Angelis I'll make an inline comment on the PR but I think we will likely want a check in there for if r.Selected != "" then return
so that SetDefaultOption doesn't become a way to SetSelected if that makes sense

that or instead of SetDefaultOption you could just have SetSelected and leave the function as is
anyhow basically saying the behavior should match the name

Alessandro De Angelis [3:00 PM]
Oh I didn’t think about that, you are right!
@stephen Houston should I change the method name to SetSelected instead and specify in the comment that it could be used to set a default option?

Stephen Houston [3:03 PM]
That's my opinion. Let's see what @andy.xyz thinks

Alessandro De Angelis [3:04 PM]
ok

Andrew Williams [3:05 PM]
That’s really a thought provoker. When do we think this will be called?
Likely it only makes sense to change if no selection was made
Should it be paired with a RequiresSelection() or similar too?

Stephen Houston [3:06 PM]
well there are other scenarios that could happen within your app
i.e. you change an option elsewhere that would then require you to select a different radio option
and so you would want to do that programmatically

Andrew Williams [3:06 PM]
If you re-select the default it will currently disable

Stephen Houston [3:09 PM]
i.e. you have radio options for default theme or custom theme... and radio options for light or dark... if default is selected light or dark is good... but if custom is selected and only has a dark, you may want to disable light and select dark programmatically. and that is a case where SetSelected is useful beyond default
(no idea if any of that is possible wrt to themes... just came up with a hypothetical)
so perhaps SetSelected with a if r.Selected == option return so you don't select twice to disable/

Andrew Williams [3:11 PM]
Oh yes, I agree that SetSelected is required and behaves in the obvious manner
But also yes SetSelected(nil) should be the only way to unselect
It I was meaning to ask about SetDefault - sorry

Stephen Houston [3:12 PM]
my point to @alessandro De Angelis is that I think there is no need for SetDefaultOption because I think SetSelected works for selecting a default AND selecting in other cases.

Andrew Williams [3:12 PM]
Ah, that is an interesting point
(Sorry on the slow uptake, I may have had a couple of cocktails this evening...)

Stephen Houston [3:13 PM]
just created your radio with options and SetSelected whatever you want the default selection to be
so instead of having two methods SetDefaultOption and SetSelected you have one that works for all cases

Andrew Williams [3:14 PM]
What do you think @alessandro De Angelis

Alessandro De Angelis [3:14 PM]
I agree with @stephen Houston

Stephen Houston [3:16 PM]
cool so update the PR and rename it to SetSelected 🙂 @andy.xyz so he will need to handle the case nil that will set Selected = "" ... any other cases? if Selected == "option" return instead of selecting twice as well?

Alessandro De Angelis [3:18 PM]
I m not sure about the nil case
O you intend “”

Stephen Houston [3:19 PM]
yes
SetSelected(option string) ... if r.Selected == option return else r.Selected = option and then Refresh should be all it needs. no additional checks for ""

Alessandro De Angelis [3:22 PM]
Yes yes
I was wondering why should I have compare a string to nil lol

Stephen Houston [3:23 PM]
an old C programmer used to NULL not ridding himself of bad habits when talking 🙂

Alessandro De Angelis [3:23 PM]
😂

Andrew Williams [3:24 PM]
Sounds good

Alessandro De Angelis [3:27 PM]
Anyway I’m going to make that change tomorrow

Andrew Williams [3:28 PM]
Cool. I am pretty busy tomorrow but will be back around coding on Thursday
Just added OnCursorChanged() to widget.Entry - I hope that helps someone (working on a TextEditor demo here)
@alessandro De Angelis the test is fine - but try to add corner cases like “” or when item is already selected etc - it’s about enforcing the contract that was basically outlined above with @stephen Houston

Alessandro De Angelis [3:35 PM]
Yeah sure!

Copy link
Member

@okratitan okratitan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me! Good job.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks!

@andydotxyz andydotxyz merged commit 2ea5c70 into fyne-io:master Apr 17, 2019
@Girbons Girbons deleted the radio-default-option branch April 17, 2019 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants