Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor gestures #677

Merged
merged 31 commits into from
Sep 16, 2021
Merged

Conversation

macdrevx
Copy link
Contributor

@macdrevx macdrevx commented Sep 14, 2021

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Update the changelog

Summary of changes

Refactors gestures for better modularity, maintainability, and testability.

Breaking Changes

  • Pan deceleration has been temporarily removed
  • TapGestureHandler.init was previously public by mistake and is now internal
  • The behavior of GestureManager.options has been updated to better reflect the isEnabled state of the associated gesture recognizers
  • The gesture recognizer properties of GestureManager are no longer Optional
  • GestureType has been redesigned so that its cases have a 1-1 relationship with the built-in gestures

Public API Additions

  • CameraState's fields are now vars instead of lets for testing purposes, and a public, memberwise initializer has been added.
  • PanScrollingMode now conforms to CaseIterable
  • GestureType now conforms to CaseIterable

Bug fixes

  • GestureManager no longer sets itself as the delegate of all gestures in MapView when its options change

Internal Refactoring

  • Generalizes CameraAnimatorMapboxMap by renaming it to MapboxMapProtocol so that it can be used throughout the SDK. SDK seems small enough that per-component dependency inversion feels unnecessary and might negatively impact binary size.
  • Removes old GestureHandlerDelegate by injecting MapboxMap and CameraAnimationsManager into each gesture handler to allow each handler to manipulate the camera directly.
  • Removes unnecessary GestureContextProvider
  • Refactors handlers and GestureManager for dependency injection and more thorough tests
  • Tidies up handlers and GestureManager and associated tests for greater consistency

@macdrevx macdrevx added breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published bug 🪲 Something is broken! labels Sep 14, 2021

delegate.pinchEnded()
mapboxMap.setCamera(
to: CameraOptions(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation feels weird here.
nit: make camera options on a different line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this indentation is advantageous for a few reasons…

  • It keeps the lines from getting too long because Xcode's indention behaviors only increase by 4 spaces per level when you indent this way.
  • It helps to avoid needing to reindent subsequent lines if, for example, you rename setCamera or CameraOptions down the road. (unlikely in this situation, but as a general rule, this is useful)
  • It provides a natural visual hierarchy of nested invocations and their associated parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a style that we are choosing to follow in our codebase? I see the arguments for it, but IMO, I think this style of indentation is harder to read and follow. It doesn't feel swifty.

I think we should standardize these things in our coding style guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should standardize these things in our coding style guide

Agreed. We don't have a standard here, so right now it's anyone's opinion.

Copy link
Contributor

@nishant-karajgikar nishant-karajgikar left a comment

Choose a reason for hiding this comment

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

🎉

@macdrevx macdrevx merged commit 0d393f0 into feature/gestures_cleanup Sep 16, 2021
@macdrevx macdrevx deleted the ah/gesture-handler-delegate-2 branch September 16, 2021 13:45
@neelmistry94 neelmistry94 mentioned this pull request Sep 21, 2021
nishant-karajgikar pushed a commit that referenced this pull request Sep 22, 2021
#### Breaking Changes

- Pan deceleration has been temporarily removed
- `TapGestureHandler.init` was previously public by mistake and is now internal
- The behavior of `GestureManager.options` has been updated to better reflect the `isEnabled` state of the associated gesture recognizers
- The gesture recognizer properties of `GestureManager` are no longer `Optional`
- `GestureType` has been redesigned so that its cases have a 1-1 relationship with the built-in gestures

#### Public API Additions

- `CameraState`'s fields are now `var`s instead of `let`s for testing purposes, and a public, memberwise initializer has been added.
- `PanScrollingMode` now conforms to `CaseIterable`
- `GestureType` now conforms to `CaseIterable`

#### Bug fixes

- GestureManager no longer sets itself as the delegate of all gestures in MapView when its options change

#### Internal Refactoring

- Generalizes `CameraAnimatorMapboxMap` by renaming it to `MapboxMapProtocol` so that it can be used throughout the SDK. SDK seems small enough that per-component dependency inversion feels unnecessary and might negatively impact binary size.
- Removes old `GestureHandlerDelegate` by injecting `MapboxMap` and `CameraAnimationsManager` into each gesture handler to allow each handler to manipulate the camera directly.
- Removes unnecessary `GestureContextProvider`
- Refactors handlers and `GestureManager` for dependency injection and more thorough tests
- Tidies up handlers and `GestureManager` and associated tests for greater consistency
nishant-karajgikar added a commit that referenced this pull request Sep 22, 2021
* remove hapticFeedbackEnabled (#663)

* remove hapticFeedbackEnabled`

* clean up changelog

* Make gestureOptions.decelarationRate the source of truth (#662)

* making gesture options decelaration rate the source of truth

* clean up changelog

* fix indentation

* Internalize UIGestureRecognizerDelegate (#669)

* seperated gesture recognizer delegate into a new class and fixed downstream breaks

* adding changelog

* swift lint and code clean up

* adding a comment

* pr comments

* Refactor gestures (#677)

#### Breaking Changes

- Pan deceleration has been temporarily removed
- `TapGestureHandler.init` was previously public by mistake and is now internal
- The behavior of `GestureManager.options` has been updated to better reflect the `isEnabled` state of the associated gesture recognizers
- The gesture recognizer properties of `GestureManager` are no longer `Optional`
- `GestureType` has been redesigned so that its cases have a 1-1 relationship with the built-in gestures

#### Public API Additions

- `CameraState`'s fields are now `var`s instead of `let`s for testing purposes, and a public, memberwise initializer has been added.
- `PanScrollingMode` now conforms to `CaseIterable`
- `GestureType` now conforms to `CaseIterable`

#### Bug fixes

- GestureManager no longer sets itself as the delegate of all gestures in MapView when its options change

#### Internal Refactoring

- Generalizes `CameraAnimatorMapboxMap` by renaming it to `MapboxMapProtocol` so that it can be used throughout the SDK. SDK seems small enough that per-component dependency inversion feels unnecessary and might negatively impact binary size.
- Removes old `GestureHandlerDelegate` by injecting `MapboxMap` and `CameraAnimationsManager` into each gesture handler to allow each handler to manipulate the camera directly.
- Removes unnecessary `GestureContextProvider`
- Refactors handlers and `GestureManager` for dependency injection and more thorough tests
- Tidies up handlers and `GestureManager` and associated tests for greater consistency

* Pan Deceleration (#692)

* update changelog

* fix changelog

* Gesture Options Cleanup (#696)

* wip

* Change double tap to zoom out to "double touch"

* Fix warning

* Lint

* Update changelog

* [run device tests]

* Use explicit self

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>
Co-authored-by: Nishant Karajgikar <nishant.karajgikar@mapbox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published bug 🪲 Something is broken!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants