TwoStateToolbarAction

[jimorc]

TwoStateToolbarAction is a toolbar action that maintains state and displays one of two icons based on the current state. Tapping the toolbar action changes the state and displays the other icon. The state is also passed to the onActivate function.
This PR implements issue #92.

[coveralls]

Pull Request Test Coverage Report for Build 11555759619

Details

• 49 of 51 (96.08%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.6%) to 48.484%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
widget/twostatetoolbaraction.go	49	51	96.08%

Totals
Change from base Build 10230812376:	0.6%
Covered Lines:	1935
Relevant Lines:	3991

---

💛 - Coveralls

[andydotxyz]

Thanks for looking at this. What I am struggling with is the naming. For example OnActivated is paired with a state parameter which doesn't really flow, would it not be OnChanged or something else that matches with other Fyne callbacks referring to state change?
The other major tripping point is TwoState0 (and 1) - this seems arbitrary and doesn't convey meaning. Confused more by the integer-like naming but boolean-like type.

I feel like this needs to review the intent / use-case so that the naming can match that expectation better.

And lastly... (sorry this is a tough one) - what does this add over and above a toolbar item where you call SetIcon when it is tapped?

[jimorc]

1. In creating TwoStateToolbarAction, I followed the code in ToolbarAction, where OnActivated is called. I guess I could change the name to OnChanged, but note that TwoStateToolbarAction uses a Button just like ToolbarAction which sets the Button.OnActivated field to Toolbar.OnActivated.
2. Why TwoState0 and TwoState1? They specify states. What do states of false and true mean? In other words, what does SetState(false) mean? There are a large number of articles and blog posts discussing boolean arguments and why they are generally bad. Alternatively, there could be two methods: SetState0 and SetState1, but that doesn't seem to match the fyne way of doing things either. The Check widget has the method SetChecked(bool), but in that case the meaning of true and false are obvious; not so with something with two states.
3. What does this toolbarAction add that calling ToolbarAction.SetIcon doesn't? State, and only having to load/set the icons once, not every time the state changes. If a standard ToolbarAction is used instead, then the state has to be kept somewhere as does the two icons. TwoStateToolbarAction captures both in a single struct.
4. I could add binding to the state as well if that is seen as useful.

I have no problem making changes, or just using this on my own projects. I thought it would be of some use to others.

Let's discuss further.

[andydotxyz]

Why TwoState0 and TwoState1? They specify states. What do states of false and true mean? In other words, what does SetState(false) mean?

I understand the naming of states to avoid a boolean - I did not mean to intend that a boolean alone was better. But you have named them as though they are integer - but they are boolean... To put it more technically this implementation looks like an N state check API where N=2. The implementation enforces there could never be any more states - but the naming implies there could.

Coming from another angle the user has no hints at all that there are other states behind this icon. If it is an "on off" state then it is acceptable because it would be indicate on or off in some way. But with N states where N-1 are not currently visible could be confusing. Should all of this actually be a segmented button where each state is visible and the current is selected?

[jimorc]

I have changed the state names to OffState and OnState, and the icons to offStateIcon and onStateIcon.

I don't think displaying both buttons at the same time is appropriate, even with one of the buttons disabled. I feel that is more confusing that displaying a single button that implies an action that can be taken.

[andydotxyz]

I have changed the state names to OffState and OnState, and the icons to offStateIcon and onStateIcon.

I don't think displaying both buttons at the same time is appropriate, even with one of the buttons disabled. I feel that is more confusing that displaying a single button that implies an action that can be taken.

Thanks for this, I think I understand now why two states was the way to go.
One last question though - given that this is just "on" and "off" why is a custom enum preferable over something like on bool?

[jimorc]

Widget has been changed to use bool rather than enum to specify state.