Adding a custom gesture recognizer for workspace-level interaction. #172

CoryDCode · 2016-09-06T18:23:53Z

This should reduce overhead for gesture recognition and allow for better
multi-touch interactions (like dragging children of currently-dragged blocks.)

This change is

CoryDCode · 2016-09-06T21:03:26Z

+@vicng +@RoboErikG

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

RoboErikG · 2016-09-06T22:49:54Z

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 2 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L2)):

//
//  BlocklyGestureRecognizer.swift

wrong header. Should be the apache license header.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 53 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L53)):

      }

      return owningBlockView(parent)

Typically something simple like this should just be done in a while loop to reduce overhead.

eg:
while !view is BlockView {
guard let view = view?.superview else {
return nil;
}
if view == _workbench.workspaceViewController {
return nil;
}
}
return view;

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 77 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L77)):

      // Check the right container - either the workspace, toolbox, or trash.
      let workspaceView: WorkspaceView
      if inToolbox {

Toolbox and Trash seem like they're mutually exclusive. Why not if inToolbox, else if inTrash, else?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 124 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L124)):

  public override func touchesMoved(touches: Set<UITouch>, withEvent event: UIEvent) {
    super.touchesMoved(touches, withEvent:event)
    for index in 0..<_touches.count {

please use the same loop pattern as you use in began and ended --> for touch in touches. It makes it easier to follow the code.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 126 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L126)):

    for index in 0..<_touches.count {
      let touch = _touches[index]
      if touches.contains(touch) {

huh...I was not expecting to find that iOS reuses the object and just keeps updating it.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 709 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L709)):

extension WorkbenchViewController {
  /**
   Removes all gesture recognizers from a block view that is part of a workspace "folder" (ie. trash

s/folder/flyout/ It looks like we'll need to do a find/replace on things. We're using flyout pretty consistently across Android and Web and should keep referring to it as flyout on iOS.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 719 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L719)):

  /**
   Copies the specified block from a folder (trash/toolbox) to the workspace.

s/folder/flyout/

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 720 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L720)):

  /**
   Copies the specified block from a folder (trash/toolbox) to the workspace.

nit ws

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 753 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L753)):

  /**
   Removes a BlockView from the trash, when moving it back to the workspace.

nit ws

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 809 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L809)):

   Pan gesture event handler for a block view inside `self.workspaceView`.
   */
  public dynamic func didRecognizeWorkspacePanGesture(gesture: BlocklyPanGestureRecognizer, touchPosition: CGPoint, blockView: BlockView, state: UIGestureRecognizerState) {

nit line length

Comments from Reviewable

CoryDCode · 2016-09-06T23:36:16Z

Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 2 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L2)):

Previously, RoboErikG wrote…

wrong header. Should be the apache license header.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 53 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L53)):

Previously, RoboErikG wrote…

Typically something simple like this should just be done in a while loop to reduce overhead.

eg:
while !view is BlockView {
guard let view = view?.superview else {
return nil;
}
if view == _workbench.workspaceViewController {
return nil;
}
}
return view;

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 77 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L77)):

Previously, RoboErikG wrote…

Toolbox and Trash seem like they're mutually exclusive. Why not if inToolbox, else if inTrash, else?

So they haven't been mutually exclusive elsewhere - previously, the trash seemed to be a toolbox. Also, the functionality is similar. But that may be a flawed understanding of the system. +@vicng?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 124 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L124)):

Previously, RoboErikG wrote…

please use the same loop pattern as you use in began and ended --> for touch in touches. It makes it easier to follow the code.

I can't, easily. We're splicing out of the _touches array, so I can't be iterating on it. I can flip flop the loop in the previous two, or aggregate a toDelete array of indices for after that loop, but both of those seemed clumsy compared to this solution. Would you prefer one of those?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 126 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L126)):

Previously, RoboErikG wrote…

huh...I was not expecting to find that iOS reuses the object and just keeps updating it.

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 709 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L709)):

Previously, RoboErikG wrote…

s/folder/flyout/ It looks like we'll need to do a find/replace on things. We're using flyout pretty consistently across Android and Web and should keep referring to it as flyout on iOS.

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 719 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L719)):

Previously, RoboErikG wrote…

s/folder/flyout/

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 720 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L720)):

Previously, RoboErikG wrote…

nit ws

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 753 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L753)):

Previously, RoboErikG wrote…

nit ws

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 809 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L809)):

Previously, RoboErikG wrote…

nit line length

Done.

Comments from Reviewable

vicng · 2016-09-07T01:34:02Z

I have some concerns with this implementation:

there is hard dependency to WorkbenchViewController, making it difficult for users to use this class if they want to implement their own version of WorkbenchViewController
In touchesBegan, the gesture has not actually started yet, but the delegate method is being fired already in that state. The user has simply touched the block. This has many adverse side effects (eg. interfering with tap gestures, copying the block immediately from the toolbox when we simply want to select/edit it, editing a block that is connected will immediately disconnect it)
I don't think BlocklyPanGestureRecognizer should be subclassing UIPanGestureRecognizer. In that implementation, the gesture would not actually transition into a ".Began" state until touchesMoved (ie. when the pan gesture has actually registered a "pan"). Calling super.touchesBegan(...) will simply transition the state into ".Possible".

I believe this implementation needs to be a completely custom gesture recognizer and handle the logic of transitioning into the right state.
4) Some edge case bugs: using two fingers to drag the same block in different directions causes weird behaviour, multitouch dragging a block to the trash doesn't work anymore

Ideally, this class should basically only deal with tracking BlockViews and all business logic should remain in WorkbenchViewController.

Here's an idea for getting around "transferring a gesture recognizer" problem:

Expose public methods in BlocklyPanGestureRecognizer to register block views that should be recognized as being touched
For the main workspace view controller, allow it to always start in the ".Possible" state in touchesBegan (regardless of what's being touched). This allows blocks that were copied into the main workspace (from the toolbox or trash) to then be registered on the main workspace's gesture recognizer after the fact by a public method in 1)
Transition to the ".Began" state in touchesMoved only if a pan is recognized on a block (either registered originally from touchesBegan or through the public method).

We can talk more in person tomorrow for ideas.

Review status: 0 of 4 files reviewed at latest revision, 28 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 11 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L11)):

import Foundation
import UIKit.UIGestureRecognizerSubclass

Nit: Remove extra line and alphabetize imports

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 14 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L14)):

import UIKit

public class BlocklyPanGestureRecognizer: UIPanGestureRecognizer {

Add class comment.

Shouldn't this be a completely custom gesture recognizer?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 17 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L17)):

  // MARK: - Properties

  private var _workbench: WorkbenchViewController

Add comments for each ivar. You can use shorthand "///" (three slashes) if each comment is 2 or less lines.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 26 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L26)):

  // MARK: - Initializer

  init(target: AnyObject?, action: Selector, workbench: WorkbenchViewController)

Add public keyword and XCode doc.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 28 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L28)):

  init(target: AnyObject?, action: Selector, workbench: WorkbenchViewController)
  {
    _workbench = workbench

Can this class be refactored without a dependency to WorkbenchViewController?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 35 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L35)):

  }

  /**

Move private method to bottom of class (under a // MARK: - Private section)

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 36 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L36)):

  /**
   Utility function for finding the first ancestor that is a BlockView.

Add backquote (`) around BlockView, so it shows up differently in XCode doc.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 39 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L39)):

   - Parameter view: The view to find an ancestor of
   - Return: The first ancestor of the UIView that is a BlockView

Add backquotes around objects.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 41 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L41)):

   - Return: The first ancestor of the UIView that is a BlockView
   */
  private func owningBlockView(view:UIView?) -> BlockView? {

Add space after view:

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 49 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L49)):

      }

      if parent == _workbench.workspaceViewController {

I think this should stop on the view this gesture recognizer is attached to, not the workbench.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 77 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L77)):

Previously, CoryDCode wrote…

So they haven't been mutually exclusive elsewhere - previously, the trash seemed to be a toolbox. Also, the functionality is similar. But that may be a flawed understanding of the system. +@vicng?

Toolbox and Trash are similar in that you can drag items out of them (which is why they share code), but they are mutually exclusive.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 99 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L99)):

      // If the touch is in the toolbox, copy the block over to the workspace first.
      if inToolbox {

This logic should be in WorkbenchViewController, not here. This class should only be dealing with recognizing block gestures, not executing business logic.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 2 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L2)):

/*
 * Copyright 2015 Google Inc. All Rights Reserved.

2016

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 66 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L66)):

  // MARK: - Super

  public override func shouldBeRequiredToFailByGestureRecognizer(

This should be set up at the Workbench level.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 96 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L96)):

      let containerView = workspaceView.scrollView.containerView
      let location = touch.locationInView(containerView)
      let hitView: UIView? = containerView.hitTest(location, withEvent: event)

Remove ": UIView?"

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 109 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L109)):

        if inTrash {
          let oldBlock = block
          block = _workbench.copyBlockToWorkspace(block!)

Avoid force unwrapping optional objects -- it's an artifact of Swift supporting Obj-C, but there are very few cases where it should be used. Clean up code here and elsewhere to remove.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 720 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L720)):

  /**
   Copies the specified block from a folder (trash/toolbox) to the workspace.

Nit: remove spaces

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 752 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L752)):

  /**
   Removes a BlockView from the trash, when moving it back to the workspace.

Nit: Add backquotes around BlockView.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 753 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L753)):

Previously, CoryDCode wrote…

Done.

If you haven't already, turn on the flag in XCode to remove trailing spaces (it doesn't work all the time though for some strange reason).

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 754 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L754)):

   Removes a BlockView from the trash, when moving it back to the workspace.

   - Parameter blockView: The BlockView to remove.

Nit: Backquotes

Comments from Reviewable

CoryDCode · 2016-09-08T23:49:17Z

Review status: 0 of 4 files reviewed at latest revision, 28 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 11 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L11)):

Previously, vicng wrote…

Nit: Remove extra line and alphabetize imports

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 14 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L14)):

Previously, vicng wrote…

Add class comment.

Shouldn't this be a completely custom gesture recognizer?

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 17 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L17)):

Previously, vicng wrote…

Add comments for each ivar. You can use shorthand "///" (three slashes) if each comment is 2 or less lines.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 26 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L26)):

Previously, vicng wrote…

Add public keyword and XCode doc.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 28 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L28)):

Previously, vicng wrote…

Can this class be refactored without a dependency to WorkbenchViewController?

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 35 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L35)):

Previously, vicng wrote…

Move private method to bottom of class (under a // MARK: - Private section)

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 36 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L36)):

Previously, vicng wrote…

Add backquote (`) around BlockView, so it shows up differently in XCode doc.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 39 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L39)):

Previously, vicng wrote…

Add backquotes around objects.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 41 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L41)):

Previously, vicng wrote…

Add space after view:

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 49 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L49)):

Previously, vicng wrote…

I think this should stop on the view this gesture recognizer is attached to, not the workbench.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 77 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L77)):

Previously, vicng wrote…

Toolbox and Trash are similar in that you can drag items out of them (which is why they share code), but they are mutually exclusive.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 99 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L99)):

Previously, vicng wrote…

This logic should be in WorkbenchViewController, not here. This class should only be dealing with recognizing block gestures, not executing business logic.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 124 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L124)):

Previously, CoryDCode wrote…

I can't, easily. We're splicing out of the _touches array, so I can't be iterating on it. I can flip flop the loop in the previous two, or aggregate a toDelete array of indices for after that loop, but both of those seemed clumsy compared to this solution. Would you prefer one of those?

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 2 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L2)):

Previously, vicng wrote…

2016

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 66 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L66)):

Previously, vicng wrote…

This should be set up at the Workbench level.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 96 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L96)):

Previously, vicng wrote…

Remove ": UIView?"

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 109 [r2] ([raw file](https://github.com/google/blockly-ios/blob/be132eb020179494eb83db94822a7ad1001592ea/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L109)):

Previously, vicng wrote…

Avoid force unwrapping optional objects -- it's an artifact of Swift supporting Obj-C, but there are very few cases where it should be used. Clean up code here and elsewhere to remove.

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 720 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L720)):

Previously, vicng wrote…

Nit: remove spaces

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 752 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L752)):

Previously, vicng wrote…

Nit: Add backquotes around BlockView.

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 753 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L753)):

Previously, vicng wrote…

If you haven't already, turn on the flag in XCode to remove trailing spaces (it doesn't work all the time though for some strange reason).

Yeah, it's already on, unfortunately.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 754 [r1] ([raw file](https://github.com/google/blockly-ios/blob/35eb6c0a7223d61097ad82d358e9eb6855e8d578/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L754)):

Previously, vicng wrote…

Nit: Backquotes

Done.

Comments from Reviewable

vicng · 2016-09-09T01:50:29Z

Looking a lot better!

Some bugs found still:

Drag a block around the workspace and the workspace doesn't remove excess scroll space
Drag a block out of the toolbox and then use your other finger to scroll the empty space in the toolbox. The toolbox ends up zooming. Basically, we need to disable zooming the toolbox and trash.
Open the Turtle demo, Turtle toolbox, and drag a block horizontally. It should just scroll the toolbox, but a block is copied out of the toolbox. You'll need to fix "WorkbenchViewController#gestureRecognizerShouldBegin".

Comments below...address those first and then I'll do another pass after cleanup.

Review status: 0 of 4 files reviewed at latest revision, 35 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 21 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L21)):

/**
 The delegate protocol for BlocklyPanGestureRecognizer.

Nit: Add backquotes.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 30 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L30)):

   Parameter gesture: The gesture calling this function.
   Parameter touchPosition: The UI space position of the touch.

To keep this consistent with the rest of the XCode doc:

"The touch position in the UIView coordinate system"

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 37 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L37)):

   */
  func blockTouched(gesture: BlocklyPanGestureRecognizer, touchPosition: CGPoint, block:BlockView,
                    state: UIGestureRecognizerState) -> BlockView;

Remove ;

It's confusing overloading state for our purposes here. It's better to create a custom enum and pass that around.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 47 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L47)):

  /// The delegate this gestureRecognizer operates on (`WorkbenchViewController` by default).
  private var _target: BlocklyPanGestureDelegate

"public weak var targetDelegate: BlocklyPanGestureDelegate?"

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 49 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L49)):

  private var _target: BlocklyPanGestureDelegate

  /// The minimum distance for the gesture recognizer to count as a pan.

Add ", in the UIView coordinate system"

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 50 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L50)):

  /// The minimum distance for the gesture recognizer to count as a pan.
  private let _minPanDistance: Float = 2.0

"public var minimumPanDistance"

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 58 [r3] ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L58)):

  private var _blocks: [BlockView]

  /**

For one or two liners, you can just use "///".

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 36 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L36)):

     might copy it into a new view.
   */
  func blockTouched(gesture: BlocklyPanGestureRecognizer, touchPosition: CGPoint, block:BlockView,

The style I've been using for delegate method signatures has been to echo the class name first.

I'd change it to this:

ie.
func blocklyPanGestureRecognizer(gesture: BlocklyPanGestureRecognizer, didTouchBlock block: BlockView, touchPosition: CGPoint, state: UIGestureRecognizerState)

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 53 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L53)):

  /// An ordered list of touches being handled by the recognizer.
  private var _touches: [UITouch]

Define this inline and remove the initialization in the init():

private var _touches = UITouch

Can _touches and _blocks be combined into a dictionary? UITouch is hashable and equatable.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 56 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L56)):

  /// An ordered list of blocks being dragged by the recognizer.
  private var _blocks: [BlockView]

Define this inline and remove the initialization in the init():

private var _blocks = BlockView

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 80 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L80)):

   */
  public init(target: BlocklyPanGestureDelegate, action: Selector, originView: UIView,
    destView: UIView, workbench: WorkbenchViewController)

Remove workbench and action parameters.

Change to targetDelegate.

Rename destView to destinationView (not a fan of abbreviations in general, unless it's incredibly obvious).

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 85 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L85)):

    _blocks = [BlockView]()
    _touches = [UITouch]()
    _originView = originView

Can self.view be used instead of _originView?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 86 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L86)):

    _touches = [UITouch]()
    _originView = originView
    _destinationView = destView

Based on the implementation below, it doesn't seem like _destinationView is needed either. All calls to locationInView could just be done on self.view, and the UITouch object could be passed back in the delegate call.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 104 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L104)):

        var block = owningBlockView(hitView)
      {
        super.touchesBegan(touches, withEvent:event)

Does super.touchesBegan() need to be fired? If so, shouldn't it be at the beginning of the method (like in touchesMoved and touchesEnded)?

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 107 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L107)):

        // Begin a new touch immediately if there is another touch being handled. Otherwise, the
        // touch will begin once a touch has been moved enough to trigger a pan.
        if state == .Began || state == .Changed {

Combine if statements into one.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 112 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L112)):

            // Start the drag.
            let touchPosition = touch.locationInView(_destinationView)
            block = _target.blockTouched(self,

I think it's awkward having the delegate return something. It's not clear why something is being returned and how the gesture recognizer would use it.

I think a better implementation would be to do this:

Create public methods to manually set mappings of UITouch objects to BlockViews
By default here, set this touch to map to the block view
In the delegate method, let it make a determination if the touch should be mapped to a new view (ie. copied BlockView). If so, then change the mapping there.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 134 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L134)):

   */
  public override func touchesMoved(touches: Set<UITouch>, withEvent event: UIEvent) {
    super.touchesMoved(touches, withEvent:event)

Same question about calling super here.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 183 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L183)):

   */
  public override func touchesEnded(touches: Set<UITouch>, withEvent event: UIEvent) {
    super.touchesEnded(touches, withEvent:event)

Same question about calling super here.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 35 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L35)):

 */
@objc(BKYWorkbenchViewController)
public class WorkbenchViewController: UIViewController, BlocklyPanGestureDelegate {

Move this protocol declaration to the extension that implements the method.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 141 [r4] ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L141)):

  /// Controller for managing the trash can workspace
  public private(set) var trashCanViewController: TrashCanViewController!

Can this be reverted back to before now?

Comments from Reviewable

CoryDCode · 2016-09-12T21:44:21Z

Scroll space wasn't getting cleared up because technically the scrollView is tracking when the scrolling fails, which is basically "any time there's a touch being handled." It was being obscured previously by the block views.

Turned off zooming for any workspace other than the main one.

Fixed the drag out functionality.

Review status: 0 of 4 files reviewed at latest revision, 35 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 21 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L21)):

Previously, vicng wrote…

Nit: Add backquotes.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 30 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L30)):

Previously, vicng wrote…

To keep this consistent with the rest of the XCode doc:

"The touch position in the UIView coordinate system"

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 37 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L37)):

Previously, vicng wrote…

Remove ;

It's confusing overloading state for our purposes here. It's better to create a custom enum and pass that around.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 47 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L47)):

Previously, vicng wrote…

"public weak var targetDelegate: BlocklyPanGestureDelegate?"

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 49 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L49)):

Previously, vicng wrote…

Add ", in the UIView coordinate system"

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 50 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L50)):

Previously, vicng wrote…

"public var minimumPanDistance"

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 58 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L58)):

Previously, vicng wrote…

For one or two liners, you can just use "///".

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 36 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L36)):

Previously, vicng wrote…

The style I've been using for delegate method signatures has been to echo the class name first.

I'd change it to this:

ie.
func blocklyPanGestureRecognizer(gesture: BlocklyPanGestureRecognizer, didTouchBlock block: BlockView, touchPosition: CGPoint, state: UIGestureRecognizerState)

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 53 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L53)):

Previously, vicng wrote…

Define this inline and remove the initialization in the init():

private var _touches = UITouch

Can _touches and _blocks be combined into a dictionary? UITouch is hashable and equatable.

The order of the arrays is currently being used, so I think it's useful to leave it this way.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 56 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L56)):

Previously, vicng wrote…

Define this inline and remove the initialization in the init():

private var _blocks = BlockView

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 80 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L80)):

Previously, vicng wrote…

Remove workbench and action parameters.

Change to targetDelegate.

Rename destView to destinationView (not a fan of abbreviations in general, unless it's incredibly obvious).

If anyone wants to listen to the GestureRecognizer functionality, they'd need to pass in an action. I'm torn on this one - I don't think that would be super useful, but it should work correctly (it should get called any time the state is set, which is why I set it to Changed when it's already Changed, for example.)

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 85 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L85)):

Previously, vicng wrote…

Can self.view be used instead of _originView?

Yeah, I think you're right here. Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 86 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L86)):

Previously, vicng wrote…

Based on the implementation below, it doesn't seem like _destinationView is needed either. All calls to locationInView could just be done on self.view, and the UITouch object could be passed back in the delegate call.

I don't think that's true. The destination view can be different from the origin view (if the gesture is attached to the toolbox/trash.) Checking the touch based on self.view will offset the new blocks (which are now children of the workspace) incorrectly.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 104 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L104)):

Previously, vicng wrote…

Does super.touchesBegan() need to be fired? If so, shouldn't it be at the beginning of the method (like in touchesMoved and touchesEnded)?

It seems prudent. I haven't been able to find a difference in functionality, but the documentation implies it's required for further gesture recognition. But yes, on the second point. Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 107 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L107)):

Previously, vicng wrote…

Combine if statements into one.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 112 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L112)):

Previously, vicng wrote…

I think it's awkward having the delegate return something. It's not clear why something is being returned and how the gesture recognizer would use it.

I think a better implementation would be to do this:

Create public methods to manually set mappings of UITouch objects to BlockViews

By default here, set this touch to map to the block view

In the delegate method, let it make a determination if the touch should be mapped to a new view (ie. copied BlockView). If so, then change the mapping there.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 134 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L134)):

Previously, vicng wrote…

Same question about calling super here.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 183 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L183)):

Previously, vicng wrote…

Same question about calling super here.

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 35 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L35)):

Previously, vicng wrote…

Move this protocol declaration to the extension that implements the method.

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 141 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L141)):

Previously, vicng wrote…

Can this be reverted back to before now?

Done.

Comments from Reviewable

vicng · 2016-09-12T23:27:47Z

Nice! Almost there :)

Found a few bugs:

If you start dragging a block and then touch the same block with another finger, as close as possible to the first touch, the second touch tries to take over dragging
Drag a block over the trash so that it grows and then drag another block on the workspace. The trash will shrink back to its original size. (This one is minor and I'd be ok if this was fixed in another CL, especially since it already existed before)
This happens rarely, but it seems that if you keep one finger touching the root block of the branch and do a lot of random connecting/disconnecting on that branch of blocks, multitouch stops working on the child blocks. I can't seem to reproduce this consistently though, so just try to keep an eye out for it. (It could in fact be that my iPad is slightly broken :S)

Review status: 0 of 4 files reviewed at latest revision, 44 unresolved discussions.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 37 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L37)):

Previously, CoryDCode wrote…

Done.

Update comments.

Rename state to touchState

And can you change to something like this:

func blocklyPanGestureRecognizer(gesture: BlocklyPanGestureRecognizer, didTouchBlock block: BlockView, touchPosition: CGPoint, touchIndex: Int, state: UIGestureRecognizerState)

Note the "didTouchBlock" part.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 80 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L80)):

Previously, CoryDCode wrote…

If anyone wants to listen to the GestureRecognizer functionality, they'd need to pass in an action. I'm torn on this one - I don't think that would be super useful, but it should work correctly (it should get called any time the state is set, which is why I set it to Changed when it's already Changed, for example.)

`action` can be removed too.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 86 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L86)):

Previously, CoryDCode wrote…

I don't think that's true. The destination view can be different from the origin view (if the gesture is attached to the toolbox/trash.) Checking the touch based on self.view will offset the new blocks (which are now children of the workspace) incorrectly.

Call super.init with a `nil` target and action. If users want to add a target/action to this recognizer, they can do it via `addTarget(...)`.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 24 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L24)):

 */
@objc
public enum BlocklyPanGestureState: Int {

Define this inside BlocklyPanGestureRecognizer and rename to "TouchState" (so as to not get confused with UIGestureRecognizer's "state")

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 25 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L25)):

@objc
public enum BlocklyPanGestureState: Int {
  case Began

Add comments for each case.

I think you can also omit the case keyword in the 2nd and 3rd cases, and separate them by commas

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 166 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L166)):

        if state == .Began {
          // If the gesture just began, begin a touch on the delegate.

This second touch should not begin immediately until it has also started panning.

I think the implementation should change so that there's a 4th touch state, called Possible, and every block touch should start in this stage.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 272 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L272)):

   - Parameter touchIndex: The index of the touch (and block) tracked by the recognizer
   */
  public func updateBlock(block: BlockView, forTouchIndex touchIndex: Int) {

I really think touch index is an implementation detail of BlocklyPanGestureRecognizer and shouldn't be exposed. It seems prone to error as well, since the same touch indices are shared by multiple entities (ie. if there was some weird multithreading situation, you can't say for certain the index you initially received is the same later). Plus, we don't want users storing this touch index for later use.

Having two data structures might make more sense here:

Keep _touches to preserve ordering
Replace _blocks with a dictionary mapping UITouches to tuples of (BlockView, TouchState)

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 274 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L274)):

  public func updateBlock(block: BlockView, forTouchIndex touchIndex: Int) {
    if (touchIndex > _blocks.count) {
      return;

Remove ;

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 944 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L944)):

   Pan gesture event handler for a block view inside `self.workspaceView`.
   */
  public dynamic func blocklyPanGestureRecognizer(gesture: BlocklyPanGestureRecognizer,

Remove dynamic

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 945 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L945)):

   */
  public dynamic func blocklyPanGestureRecognizer(gesture: BlocklyPanGestureRecognizer,
                                                  touchPosition: CGPoint, touchIndex: Int, block: BlockView, state: BlocklyPanGestureState)

line length

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 1032 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L1032)):

    if let panGestureRecognizer = gestureRecognizer as? BlocklyPanGestureRecognizer
    {
      if gestureRecognizer.view == toolboxCategoryViewController.view {

Move this to a where clause in the if let statement

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 1080 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L1080)):

    -> Bool
  {
    return true

No conditions?

Blockly/Code/UI/Views/WorkspaceView.swift, line 64 at r5 (raw file):

  public var scrollIntoViewEdgeInsets = EdgeInsets(20, 20, 100, 20)

  /// Enables/disables the zooming of a workspace

Mention that it defaults to false.

We'll have to add this to all our documentation soon, sorry :)

Comments from Reviewable

CoryDCode · 2016-09-13T01:29:04Z

Spoke offline - I've seen the first bug before, but I think it's just the device erroneously thinking the first touch has moved, rather than an error with this implementation.

Fixed the trash can highlight with multiple drags (and the subsequent bug where the trash can reset when dropping a block not currently touching the trash can.)

I haven't seen that yet, but I'll keep testing and see what I can figure out.

Review status: 0 of 4 files reviewed at latest revision, 44 unresolved discussions.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 944 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L944)):

Previously, vicng wrote…

Remove dynamic

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 945 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L945)):

Previously, vicng wrote…

line length

Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 1032 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L1032)):

Previously, vicng wrote…

Move this to a where clause in the if let statement

Ah, neat. Done.

Blockly/Code/UI/View Controllers/WorkbenchViewController.swift, line 1080 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/WorkbenchViewController.swift#L1080)):

Previously, vicng wrote…

No conditions?

Seems it's no longer needed. It was needed for buttons and such to be recognized, but it seems I've fixed that along the way, as well. Removed.

Blockly/Code/UI/Views/WorkspaceView.swift, line 64 at r5 (raw file):

Previously, vicng wrote…

Mention that it defaults to false.

We'll have to add this to all our documentation soon, sorry :)

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 37 at r3 ([raw file](https://github.com/google/blockly-ios/blob/0d532ed64208134096a18e94acbd532b2e178d15/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L37)):

Previously, vicng wrote…

Update comments.

Rename state to touchState

And can you change to something like this:

func blocklyPanGestureRecognizer(gesture: BlocklyPanGestureRecognizer, didTouchBlock block: BlockView, touchPosition: CGPoint, touchIndex: Int, state: UIGestureRecognizerState)

Note the "didTouchBlock" part.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 80 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L80)):

Previously, vicng wrote…

action can be removed too.

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 86 at r4 ([raw file](https://github.com/google/blockly-ios/blob/187aec6736b0a2ce8068bf1cb3a0ff1b1af4f36c/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L86)):

Previously, vicng wrote…

Call super.init with a nil target and action. If users want to add a target/action to this recognizer, they can do it via addTarget(...).

Fair enough. Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 24 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L24)):

Previously, vicng wrote…

Define this inside BlocklyPanGestureRecognizer and rename to "TouchState" (so as to not get confused with UIGestureRecognizer's "state")

Done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 25 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L25)):

Previously, vicng wrote…

Add comments for each case.

I think you can also omit the case keyword in the 2nd and 3rd cases, and separate them by commas

There are some differences between the Swift specification for enums and the objective-C specification. In order to force each one to have an integer-backed value (which is needed to allow for it to be used in objective c,) each one needs to be specified by case. Also, done.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 166 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L166)):

Previously, vicng wrote…

This second touch should not begin immediately until it has also started panning.

I think the implementation should change so that there's a 4th touch state, called Possible, and every block touch should start in this stage.

So, I disagree, but I might be misunderstanding your comment.

The first half of this function checks to see if the gesture needs to begin. If it does, execution will trickle down to this part of the function, on the first touch. This will then call through to inform the delegate that it has begun. So this chunk of code is the first touch. (Or at least the first touch to recognize the gesture.)

HOWEVER, the second touch does begin when it is touched, not when it is panning, and the reason is that it more closely models the behavior of the gesture recognizer. We don't start the gesture until the pan of the first block because you could, for example, tap a block to edit it. You CANNOT do that with the second block. So, we start the drag immediately on subsequent blocks, because it would be more confusing in my mind to invisibly fail. This way, you get a highlight around the block when you tap the second one. It's more obvious what's happening.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 272 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L272)):

Previously, vicng wrote…

I really think touch index is an implementation detail of BlocklyPanGestureRecognizer and shouldn't be exposed. It seems prone to error as well, since the same touch indices are shared by multiple entities (ie. if there was some weird multithreading situation, you can't say for certain the index you initially received is the same later). Plus, we don't want users storing this touch index for later use.

Having two data structures might make more sense here:

Keep _touches to preserve ordering

Replace _blocks with a dictionary mapping UITouches to tuples of (BlockView, TouchState)

I agree with the point about indices, and have changed the surface of that function. As mentioned above, I think tracking the state for each touch would be modeling the system incorrectly, so I would rather not store them as blockview/state tuples.

Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift, line 274 at r5 ([raw file](https://github.com/google/blockly-ios/blob/2452e6eeb1b26c92f901ddae835bc8160d4f8767/Blockly/Code/UI/View Controllers/BlocklyPanGestureRecognizer.swift#L274)):

Previously, vicng wrote…

Remove ;

Been doing too much Unity in my free time, apparently. Done.

Comments from Reviewable

vicng · 2016-09-13T04:42:10Z

Found a couple more bugs:

If you start touching two blocks at the same time and then try panning both simultaneously, only one of the pans is recognized.
Begin dragging a block with one finger. Put a second finger on the same block. Take your first finger off the block. The block jumps to where the second finger is positioned, as opposed to position gracefully to the second touch. (This one may require a bit of code to fix, I'd be fine if it was a follow up TODO.)

Sorry for being so tough on this CL. Touch is VERY IMPORTANT to user interaction (ALL CAPS!), so I just want to make sure we get it working 100%.

Review status: 0 of 4 files reviewed at latest revision, 38 unresolved discussions.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 37 at r3 (raw file):

Previously, CoryDCode wrote…

Done.

didTouchBlock should be the 2nd parameter.

Add a space between block: BlockView

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 86 at r4 (raw file):

Previously, CoryDCode wrote…

Fair enough. Done.

I might be missing something here, but I still don't think you need _destinationView.

There are only two ways it's being used below:

To calculate the distance of the pan, when determining if transitioning between .Possible and .Began.
To calculate the touch position relative to _destinationView, which only gets passed to the delegate method and is not used elsewhere in this class.

Fixes for both cases would be:

Calculate the distance relative to self.view.
ie. let previousPosition = touch.previousLocationInView(self.view)

(omit self. in the actual impl)

The UITouch object would be a better object to pass back to the delegate. Let the delegate figure out which view it wants the touch position relative to.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 25 at r5 (raw file):

Previously, CoryDCode wrote…

There are some differences between the Swift specification for enums and the objective-C specification. In order to force each one to have an integer-backed value (which is needed to allow for it to be used in objective c,) each one needs to be specified by case. Also, done.

Right, I should have pointed you to a better example earlier. Take a look at Input.swift on how to define an enum that's compatible for both. The current code is not great because `TouchState` will simply create a class called `TouchState` on the ObjC side, which can be dangerous since there's no prefix.

Basically you have to name the class with the name you'd want it outputted as in ObjC, because adding a custom name via the @objc annotation doesn't work properly for enums (although maybe that's been fixed in the latest version of Swift). It should be BKYBlocklyPanGestureRecognizerTouchState.

After that, then you should add a shorter typealias for that in Swift.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 166 at r5 (raw file):

Previously, CoryDCode wrote…

So, I disagree, but I might be misunderstanding your comment.

The first half of this function checks to see if the gesture needs to begin. If it does, execution will trickle down to this part of the function, on the first touch. This will then call through to inform the delegate that it has begun. So this chunk of code is the first touch. (Or at least the first touch to recognize the gesture.)

HOWEVER, the second touch does begin when it is touched, not when it is panning, and the reason is that it more closely models the behavior of the gesture recognizer. We don't start the gesture until the pan of the first block because you could, for example, tap a block to edit it. You CANNOT do that with the second block. So, we start the drag immediately on subsequent blocks, because it would be more confusing in my mind to invisibly fail. This way, you get a highlight around the block when you tap the second one. It's more obvious what's happening.

Hmm actually, I do like the behaviour when it's on the main workspace.

The problem more lies with initiating a multitouch inside the toolbox. If you start dragging a block out of the toolbox, accidentally touching any block in the toolbox immediately copies it into the workspace. This doesn't seem correct.

Maybe it should be able to support both ways? (You can mark this as a TODO to revisit later if you'd like.)

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 29 at r6 (raw file):

   typically copying the view onto a new workspace.

   Parameter gesture: The gesture calling this function.

Update comments.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 73 at r6 (raw file):

  /// The minimum distance for the gesture recognizer to count as a pan, in the UIView coordinate
  /// system.
  public var minPanDistance: Float = 2.0

minimumPanDistance

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 76 at r6 (raw file):

  /// The delegate this gestureRecognizer operates on (`WorkbenchViewController` by default).
  public weak var targetDelegate: BlocklyPanGestureDelegate!

This should be an optional (?)

Any time you feel the urge to use "!" for variables, don't :)

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 100 at r6 (raw file):

   Called when touches begin on the workspace.
   */
  public override func touchesBegan(touches: Set<UITouch>, withEvent event: UIEvent) {

Dumb question: Do you ever need to explicitly put the state of the gesture in .Possible or is it done automatically?

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 106 at r6 (raw file):

      // If the hit tested view is not an ancestor of a block, cancel the touch(es).
      if let hitView = self.view!.hitTest(location, withEvent: event),

Rewrite to avoid force unwrapping self.view

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 151 at r6 (raw file):

          continue
        }
      } else if state == .Began || state == .Changed {

Why is state == .Changed here?

Comments from Reviewable

CoryDCode · 2016-09-13T23:21:51Z

Fixed the first by splitting the gesture recognizing bookkeeping further from the delegate.

Added #175 for the second.

And it's okay. I'd be lying if I said it wasn't frustrating, but that's not your fault. ;)

Review status: 0 of 4 files reviewed at latest revision, 38 unresolved discussions.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 37 at r3 (raw file):

Previously, vicng wrote…

didTouchBlock should be the 2nd parameter.

Add a space between block: BlockView

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 86 at r4 (raw file):

Previously, vicng wrote…

I might be missing something here, but I still don't think you need _destinationView.

There are only two ways it's being used below:

To calculate the distance of the pan, when determining if transitioning between .Possible and .Began.

To calculate the touch position relative to _destinationView, which only gets passed to the delegate method and is not used elsewhere in this class.

Fixes for both cases would be:

Calculate the distance relative to self.view.
ie. let previousPosition = touch.previousLocationInView(self.view)

(omit self. in the actual impl)

The UITouch object would be a better object to pass back to the delegate. Let the delegate figure out which view it wants the touch position relative to.

That's fair. Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 25 at r5 (raw file):

Previously, vicng wrote…

Right, I should have pointed you to a better example earlier. Take a look at Input.swift on how to define an enum that's compatible for both. The current code is not great because TouchState will simply create a class called TouchState on the ObjC side, which can be dangerous since there's no prefix.

Basically you have to name the class with the name you'd want it outputted as in ObjC, because adding a custom name via the @objc annotation doesn't work properly for enums (although maybe that's been fixed in the latest version of Swift). It should be BKYBlocklyPanGestureRecognizerTouchState.

After that, then you should add a shorter typealias for that in Swift.

Ah, I see. Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 166 at r5 (raw file):

Previously, vicng wrote…

Hmm actually, I do like the behaviour when it's on the main workspace.

The problem more lies with initiating a multitouch inside the toolbox. If you start dragging a block out of the toolbox, accidentally touching any block in the toolbox immediately copies it into the workspace. This doesn't seem correct.

Maybe it should be able to support both ways? (You can mark this as a TODO to revisit later if you'd like.)

Added maximumTouches to support both.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 29 at r6 (raw file):

Previously, vicng wrote…

Update comments.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 73 at r6 (raw file):

Previously, vicng wrote…

minimumPanDistance

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 76 at r6 (raw file):

Previously, vicng wrote…

This should be an optional (?)

Any time you feel the urge to use "!" for variables, don't :)

Huh. I could've sworn I literally copied this from your previous comment. No idea how it wound up like that. Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 100 at r6 (raw file):

Previously, vicng wrote…

Dumb question: Do you ever need to explicitly put the state of the gesture in .Possible or is it done automatically?

Nope, that happens automatically. There is a reset function, but that happens right before it gets put back in .Possible.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 106 at r6 (raw file):

Previously, vicng wrote…

Rewrite to avoid force unwrapping self.view

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 151 at r6 (raw file):

Previously, vicng wrote…

Why is state == .Changed here?

Setting the state to .Changed fires delegate methods, and should be done even if the state is already changed. (https://developer.apple.com/library/content/documentation/EventHandling/Conceptual/EventHandlingiPhoneOS/Art/gr_state_transitions_2x.png)

Comments from Reviewable

vicng · 2016-09-14T01:00:19Z

As discussed offline, it looks like there's only one remaining bug that needs to be addressed. Steps to reproduce:

Connect two blocks in the workspace and move the parent block to the bottom-right corner.
Then drag the parent block far away from its current position (ie. top-left corner) and try multitouch disconnecting the child block. This doesn't work.

Review status: 0 of 4 files reviewed at latest revision, 47 unresolved discussions.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 151 at r6 (raw file):

Previously, CoryDCode wrote…

Setting the state to .Changed fires delegate methods, and should be done even if the state is already changed. (https://developer.apple.com/library/content/documentation/EventHandling/Conceptual/EventHandlingiPhoneOS/Art/gr_state_transitions_2x.png)

Oh that makes sense. Can you add a comment about this?

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 31 at r7 (raw file):

   Parameter gesture: The gesture calling this function.
   Parameter touch: The `UITouch` hitting the block.
   Parameter didTouchBlock: The `BlockView` being touched.

This should be the variable here, instead of the method signature (ie. block).

Also, move this line above touch.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 32 at r7 (raw file):

   Parameter touch: The `UITouch` hitting the block.
   Parameter didTouchBlock: The `BlockView` being touched.
   Parameter touchState: The `UIGestureRecognizerState` for this individual touch.

TouchState

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 57 at r7 (raw file):

  @objc
  public enum BKYBlocklyPanGestureRecognizerTouchState: Int {
    // Specifies an individual touch has just begun, changed, or ended on a `BlockView`

I think you have to separate comments for each for Jazzy to work properly.

Also use "///" instead of "//"

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 63 at r7 (raw file):

  /// Maximum number of touches handled by the recognizer
  public var maximumTouches = Int.max

Discussed offline, this is a good interim solution for fixing the toolbox problem. Can you add a follow up TODO/issue to explore multi-touch dragging out of the toolbox?

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 106 at r7 (raw file):

        _blocks.append(block)

        guard let delegate = targetDelegate else {

You can remove this guard and just reference it below as delegate?.. That's pretty standard practice since delegate is an optional ivar.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 173 at r7 (raw file):

          }

          guard let delegate = targetDelegate else {

Same thing here.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 185 at r7 (raw file):

    } else {
      for touch in touches {

Nit: Remove extra line.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 194 at r7 (raw file):

          }

          guard let delegate = targetDelegate else {

Same here.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 226 at r7 (raw file):

        // TODO:(#175) Fix blocks jumping from touch to touch when one block is hit by two touches.

        guard let delegate = targetDelegate else {

Here too.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 339 at r7 (raw file):

      }

      if currentView == view {

This should be self.view, right? Or else it's now comparing against the view passed in through the parameter.

Comments from Reviewable

CoryDCode · 2016-09-14T18:38:42Z

Fixed in ZIndexedGroupView. I'm not 100% convinced this is the right place, but the containerView is the only place that uses this class, and I want to fix the hit testing of the containerView.

Review status: 0 of 5 files reviewed at latest revision, 47 unresolved discussions.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 151 at r6 (raw file):

Previously, vicng wrote…

Oh that makes sense. Can you add a comment about this?

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 31 at r7 (raw file):

Previously, vicng wrote…

This should be the variable here, instead of the method signature (ie. block).

Also, move this line above touch.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 32 at r7 (raw file):

Previously, vicng wrote…

TouchState

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 57 at r7 (raw file):

Previously, vicng wrote…

I think you have to separate comments for each for Jazzy to work properly.

Also use "///" instead of "//"

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 63 at r7 (raw file):

Previously, vicng wrote…

Discussed offline, this is a good interim solution for fixing the toolbox problem. Can you add a follow up TODO/issue to explore multi-touch dragging out of the toolbox?

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 106 at r7 (raw file):

Previously, vicng wrote…

You can remove this guard and just reference it below as delegate?.. That's pretty standard practice since delegate is an optional ivar.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 173 at r7 (raw file):

Previously, vicng wrote…

Same thing here.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 185 at r7 (raw file):

Previously, vicng wrote…

Nit: Remove extra line.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 194 at r7 (raw file):

Previously, vicng wrote…

Same here.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 226 at r7 (raw file):

Previously, vicng wrote…

Here too.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 339 at r7 (raw file):

Previously, vicng wrote…

This should be self.view, right? Or else it's now comparing against the view passed in through the parameter.

Done.

Comments from Reviewable

vicng · 2016-09-14T22:47:18Z

Yeah, it's probably not the "correct" place, but we can leave it there until we find we're using it somewhere else.

Minor nits, but

Thanks for your hard work on this!

Review status: 0 of 5 files reviewed at latest revision, 50 unresolved discussions, some commit checks failed.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 63 at r7 (raw file):

Previously, CoryDCode wrote…

Done.

After playing around with it some more, I think it's better to not have the toolbox/trash limited by maximumTouches (we should still find a better solution later tho).

Can you leave maximumTouches to Int.max for the toolbox and trash? Sorry about that.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 25 at r8 (raw file):

public protocol BlocklyPanGestureDelegate: class {
  /**
   The callback that's called when the BlocklyPanGestureRecognizer detects a valid block pan. Note:

Nit: Add backquotes.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 60 at r8 (raw file):

    case Began = 0,
    /// Specifies an individual touch has just changed on a `BlockView`
         Changed,

Nit: Indent 2 spaces

Blockly/Code/UI/Views/ZIndexedGroupView.swift, line 97 at r8 (raw file):

  }

  /**

Can you add section marks to this file and move this to the right section?

The ordering is listed in go/blockly-style (https://docs.google.com/document/d/1839QscdvNxCyovYJfZs8gYFku36ZiOyTeDEmb1eHuzU/edit#heading=h.q4uvggxyl1s0)

// MARK: - Properties

// MARK: - Super

// MARK: - Public

// MARK: - Private

Comments from Reviewable

CoryDCode · 2016-09-14T23:22:59Z

Review status: 0 of 6 files reviewed at latest revision, 50 unresolved discussions, some commit checks failed.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 63 at r7 (raw file):

Previously, vicng wrote…

After playing around with it some more, I think it's better to not have the toolbox/trash limited by maximumTouches (we should still find a better solution later tho).

Can you leave maximumTouches to Int.max for the toolbox and trash? Sorry about that.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 25 at r8 (raw file):

Previously, vicng wrote…

Nit: Add backquotes.

Done.

Blockly/Code/UI/BlocklyPanGestureRecognizer.swift, line 60 at r8 (raw file):

Previously, vicng wrote…

Nit: Indent 2 spaces

Done.

Blockly/Code/UI/Views/ZIndexedGroupView.swift, line 97 at r8 (raw file):

Previously, vicng wrote…

Can you add section marks to this file and move this to the right section?

The ordering is listed in go/blockly-style (https://docs.google.com/document/d/1839QscdvNxCyovYJfZs8gYFku36ZiOyTeDEmb1eHuzU/edit#heading=h.q4uvggxyl1s0)

// MARK: - Properties

// MARK: - Super

// MARK: - Public

// MARK: - Private

Done.

Comments from Reviewable

This should reduce overhead for gesture recognition and allow for better multi-touch interactions (like dragging children of currently-dragged blocks.)

CoryDCode force-pushed the cdCustomGestures branch 2 times, most recently from dd2d1a8 to 35eb6c0 Compare September 6, 2016 21:02

CoryDCode assigned RoboErikG and vicng Sep 6, 2016

CoryDCode force-pushed the cdCustomGestures branch 2 times, most recently from fae6349 to 7c19576 Compare September 6, 2016 23:34

CoryDCode force-pushed the cdCustomGestures branch from 7c19576 to be132eb Compare September 6, 2016 23:59

CoryDCode force-pushed the cdCustomGestures branch 2 times, most recently from 153cdba to 0d532ed Compare September 8, 2016 23:48

CoryDCode force-pushed the cdCustomGestures branch from 0d532ed to 187aec6 Compare September 8, 2016 23:53

CoryDCode force-pushed the cdCustomGestures branch from 187aec6 to 78798ab Compare September 12, 2016 21:33

CoryDCode force-pushed the cdCustomGestures branch from 78798ab to 2452e6e Compare September 12, 2016 21:56

CoryDCode force-pushed the cdCustomGestures branch from 2452e6e to af494e6 Compare September 13, 2016 01:27

CoryDCode force-pushed the cdCustomGestures branch from af494e6 to 00371a3 Compare September 13, 2016 23:10

CoryDCode force-pushed the cdCustomGestures branch 2 times, most recently from dfa7927 to 1f15457 Compare September 14, 2016 18:38

CoryDCode force-pushed the cdCustomGestures branch from 1f15457 to 1e38e64 Compare September 14, 2016 21:55

CoryDCode force-pushed the cdCustomGestures branch 3 times, most recently from fd9cff2 to 7e07794 Compare September 14, 2016 23:16

Adding a custom gesture recognizer for workspace-level interaction.

0e72640

This should reduce overhead for gesture recognition and allow for better multi-touch interactions (like dragging children of currently-dragged blocks.)

CoryDCode force-pushed the cdCustomGestures branch from 7e07794 to 0e72640 Compare September 14, 2016 23:29

CoryDCode merged commit 5e5f805 into master Sep 14, 2016

CoryDCode deleted the cdCustomGestures branch September 14, 2016 23:33

This was referenced Sep 14, 2016

Fixing multi-drag so you can drag the children of currently dragging … #153

Closed

Multitouch block dragging doesn't work for child blocks #122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a custom gesture recognizer for workspace-level interaction. #172

Adding a custom gesture recognizer for workspace-level interaction. #172

CoryDCode commented Sep 6, 2016 •

edited by RoboErikG

Loading

CoryDCode commented Sep 6, 2016

RoboErikG commented Sep 6, 2016

CoryDCode commented Sep 6, 2016

vicng commented Sep 7, 2016

CoryDCode commented Sep 8, 2016

vicng commented Sep 9, 2016

CoryDCode commented Sep 12, 2016

vicng commented Sep 12, 2016

CoryDCode commented Sep 13, 2016

vicng commented Sep 13, 2016

CoryDCode commented Sep 13, 2016

vicng commented Sep 14, 2016

CoryDCode commented Sep 14, 2016

vicng commented Sep 14, 2016

CoryDCode commented Sep 14, 2016

Adding a custom gesture recognizer for workspace-level interaction. #172

Adding a custom gesture recognizer for workspace-level interaction. #172

Conversation

CoryDCode commented Sep 6, 2016 • edited by RoboErikG Loading

CoryDCode commented Sep 6, 2016

RoboErikG commented Sep 6, 2016

CoryDCode commented Sep 6, 2016

vicng commented Sep 7, 2016

CoryDCode commented Sep 8, 2016

vicng commented Sep 9, 2016

CoryDCode commented Sep 12, 2016

vicng commented Sep 12, 2016

CoryDCode commented Sep 13, 2016

vicng commented Sep 13, 2016

CoryDCode commented Sep 13, 2016

vicng commented Sep 14, 2016

CoryDCode commented Sep 14, 2016

vicng commented Sep 14, 2016

CoryDCode commented Sep 14, 2016

CoryDCode commented Sep 6, 2016 •

edited by RoboErikG

Loading