Skip to content

Usage example for toggle group - #2

Open
nader-eloshaiker wants to merge 5 commits into
hanjoes:masterfrom
nader-eloshaiker:master
Open

Usage example for toggle group#2
nader-eloshaiker wants to merge 5 commits into
hanjoes:masterfrom
nader-eloshaiker:master

Conversation

@nader-eloshaiker

@nader-eloshaiker nader-eloshaiker commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

No description provided.

@nader-eloshaiker

nader-eloshaiker commented Jan 12, 2018

Copy link
Copy Markdown
Contributor Author

@hanjoes thought it might be a good idea to show how the grouping would work

@hanjoes

hanjoes commented Jan 12, 2018

Copy link
Copy Markdown
Owner

yep, once i added the tests I will put that in the demo

@hanjoes

hanjoes commented Jan 13, 2018

Copy link
Copy Markdown
Owner

While I'm trying to build the demo, I had some more thoughts about the toggle feature. I want to know your opinion:

  1. I want to update the isToggleOn property to be isToggled and make it read-only . To change the toggle state, we instead introduce a toggle() method. By default a toggle button is toggled off, and inside toggle() we implement logic to disable toggling off for a button whose toggleGroup is not empty. The toggle() method is essentially the same as the current invokeToggleBehavior() method (maybe with some small tweaks).

  2. I'd like to introduce a "type" or "behavior" property to indicate whether it's a toggle button or normal button, this is also for better user interface and scalability. The current boolean solution (isToggleMode) will soon get confusing if there is another type of button added. (for e.g.: check box).

@nader-eloshaiker please let me know your thoughts. (I feel we can hold off 2, but 1 is more important to me at the moment.)

@hanjoes

hanjoes commented Jan 14, 2018

Copy link
Copy Markdown
Owner

Also the current way of adding each other to the toggle group will introduce strong reference cycles if not used properly , also adding mutual dependency doesn't feel right if we have a lot of buttons while using some centralized store for toggle groups can potentially make the maintenance a lot easier.

@nader-eloshaiker

nader-eloshaiker commented Jan 14, 2018

Copy link
Copy Markdown
Contributor Author

Point 1 sounds like a good approach, however, with point 2, you will need a way for the button to know it is associated with a toggle group to ensure certain behaviour, ie perhaps not trigger toggle off behaviour. Perhaps start a branch and we can work on this together?

@nader-eloshaiker

nader-eloshaiker commented Jan 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

With point 2, perhaps we should consider creating an entirely new concrete class called SKSpriteButtonGroup and attach action listerners to that as well of the toggle action on the button itself. eg

init() {
   let radioButtonGroup = SKSpriteButtonGroup()
   radioButtonGroup.addButton(button1)
   radioButtonGroup.addButton(button2)
   radioButton.addToggleAction(self.radioSelectionChanged)
}

func radioSelectionChanged(target:SKSpriteButton) {
  if target == button1 {
    doSomethingFun()
  } else if target == button2 {
    doSomethingBoring()
  }
}

That way, if the dev want to create a plain old toggle button, he can add action listeners to the button, but if the dev wants to create a group, then he/she only needs to assign one listener.

Doing this, get around having to check if the button belongs to a group and not calling toggle off actions as the dev doesn't have to worry about it.

@nader-eloshaiker

nader-eloshaiker commented Jan 15, 2018

Copy link
Copy Markdown
Contributor Author

Thinking about this some more, you will still need, at the very least, a weak reference to the button group object to be able to call the group actions on a button touch event.

@hanjoes

hanjoes commented Jan 15, 2018

Copy link
Copy Markdown
Owner

I like the SKSpriteButtonGroup idea. actually had some local implementation after my comments yesterday (with exactly the same name!) along with an "Unowned" wrapper (I prefer unowned here since it doesn't make sense for an empty wrapper in the button use case and we are not exposing that wrapper to users.) Here are some simple code example:

struct SKUnowned<T: AnyObject & Hashable>: Hashable  {
    var hashValue: Int {
        return value.hashValue * 31
    }
    
    static func ==(lhs: SKUnowned<T>, rhs: SKUnowned<T>) -> Bool {
        return lhs == rhs
    }
    
    unowned var value: T
}


class SKSpriteButtonGroup {
    
    public var buttons = Set<SKUnowned<SKSpriteButton>>()
    
    public func add(button: SKSpriteButton) {
        buttons.insert(SKUnowned(value: button))
    }
    
    public func remove(button: SKSpriteButton) {
        buttons.remove(SKUnowned(value: button))
    }
}

This button group should really be general, but it serves features like toggling.

For group toggling to work, we should let user creates a group and adds the buttons to group (during insertion we assign the group back to each button's "group" property). The toggle() method should be aware of the group property in it's implementation.

I will create a remote branch. Currently we have 3 main things to do:

  1. create a toggle() method and refactor the code
  2. create a SKSpriteButtonGroup which is a general group concept.
  3. refactor the toggle() method to use the group concept.

@nader-eloshaiker

nader-eloshaiker commented Jan 15, 2018

Copy link
Copy Markdown
Contributor Author

How do you feel about the idea of adding a handler to the SKSpriteButtonGroup class

@hanjoes

hanjoes commented Jan 15, 2018

Copy link
Copy Markdown
Owner

If it's not absolutely needed at the moment for implementing toggle group, we don't need to add it immediately.

@nader-eloshaiker

nader-eloshaiker commented Jan 15, 2018

Copy link
Copy Markdown
Contributor Author

Definitely not a must have, we can add it in at a later time if we still think it is a good idea.

@hanjoes

hanjoes commented Jan 15, 2018

Copy link
Copy Markdown
Owner

cool, i can work on 2 and maybe you can finishing up toggling with 1 and 3?

@nader-eloshaiker

nader-eloshaiker commented Jan 15, 2018

Copy link
Copy Markdown
Contributor Author

Great :)

@hanjoes

hanjoes commented Jan 15, 2018

Copy link
Copy Markdown
Owner

added toggle-group remote branch :)

@nader-eloshaiker

nader-eloshaiker commented Jan 15, 2018

Copy link
Copy Markdown
Contributor Author

I'll need permissions to write to it (saves me having to manually sync my fork)

@nader-eloshaiker

nader-eloshaiker commented Jan 15, 2018

Copy link
Copy Markdown
Contributor Author

I'm happy for you to be the gate keeper for merges into master.

@hanjoes

hanjoes commented Jan 15, 2018

Copy link
Copy Markdown
Owner

sure, let me add you to collaborator

@nader-eloshaiker

nader-eloshaiker commented Mar 24, 2018 via email

Copy link
Copy Markdown
Contributor Author

@hanjoes

hanjoes commented Mar 25, 2018 via email

Copy link
Copy Markdown
Owner

@nader-eloshaiker

nader-eloshaiker commented Mar 25, 2018 via email

Copy link
Copy Markdown
Contributor Author

@nader-eloshaiker

nader-eloshaiker commented Apr 7, 2018 via email

Copy link
Copy Markdown
Contributor Author

@hanjoes

hanjoes commented Apr 7, 2018

Copy link
Copy Markdown
Owner

Hey! Got distracted by something else. Sorry, i will take a look this weekend. It does seem to be a big refactoring.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nader-eloshaiker @hanjoes