Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[camera]refactor flash mode and add unit test #4695

Merged
merged 4 commits into from
Jan 27, 2022
Merged

[camera]refactor flash mode and add unit test #4695

merged 4 commits into from
Jan 27, 2022

Conversation

hellohuanlin
Copy link
Contributor

@hellohuanlin hellohuanlin commented Jan 25, 2022 •
edited
Loading

Breaking up CameraPlugin.m is part of optional cleanups discussed in the proposal. The monolithic CameraPlugin.m makes it hard to unit test other sub-tasks, so we want to do this clean up sooner.

This PR moves FlashMode type and its related util functions into a separate file. Also added unit tests for these util functions.

No version change: just moving code

Issue here: flutter/flutter#96429

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@hellohuanlin hellohuanlin force-pushed the camera_refactor_flash_mode branch 3 times, most recently from 224bbf0 to b948452 Compare January 25, 2022 23:58
@hellohuanlin hellohuanlin marked this pull request as ready for review January 26, 2022 00:00
@hellohuanlin hellohuanlin requested review from stuartmorgan, jmagman and cyanglaz and removed request for bparrishMines January 26, 2022 00:00
jmagman
jmagman suggested changes Jan 26, 2022
View reviewed changes
@hellohuanlin
Copy link
Contributor Author

hellohuanlin commented Jan 26, 2022 •
edited
Loading

No version change: just moving code

@stuartmorgan Just double check - are you fine with not bumping the version here? Or do you think we should bump it because we added unit tests? There will be multiple similar PRs coming up.

@hellohuanlin hellohuanlin requested a review from jmagman January 26, 2022 20:37
jmagman
jmagman approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM with naming nit.
Thanks @hellohuanlin!

jmagman
jmagman reviewed Jan 26, 2022
View reviewed changes
packages/camera/camera/ios/Classes/CameraPlugin.m Outdated
@@ -10,6 +10,7 @@
#import <CoreMotion/CoreMotion.h>
#import <libkern/OSAtomic.h>
#import <uuid/uuid.h>
#import "FLTFlashMode.h"
Copy link
Member

Choose a reason for hiding this comment

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

I think FlashMode.h as you had it was better, there's no reason to prefix header names (that I'm aware of), even though a few other files have done so.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason in general is, per style guide:

File names should reflect the name of the class implementation that they contain—including case.

Most files are classes, and thus have a prefix because the class does.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, that is the pattern, and Apple also prefixes their header names. Hard to keep it all straight. 🙂

@hellohuanlin hellohuanlin added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jan 26, 2022
@hellohuanlin hellohuanlin force-pushed the camera_refactor_flash_mode branch from 14babcd to 672a77e Compare January 26, 2022 23:32
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jan 27, 2022
@hellohuanlin
Copy link
Contributor Author

This is strange. It is conflicting with #4692, which is not landed to main yet.

@jmagman
Copy link
Member

jmagman commented Jan 27, 2022

This is strange. It is conflicting with #4692, which is not landed to main yet.

It's conflicting with #4519

@hellohuanlin
Copy link
Contributor Author

This is strange. It is conflicting with #4692, which is not landed to main yet.

Ah right. my mistake

@hellohuanlin hellohuanlin force-pushed the camera_refactor_flash_mode branch from 672a77e to bad2437 Compare January 27, 2022 00:49
@hellohuanlin hellohuanlin added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jan 27, 2022
@fluttergithubbot fluttergithubbot merged commit 3cc18d9 into flutter:main Jan 27, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 27, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2022
@engine-flutter-autoroll
3cc18d9 [camera]refactor flash mode and add unit test (flutter/plugin…
f5c5b1c 
…s#4695)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 27, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2022
@engine-flutter-autoroll
3cc18d9 [camera]refactor flash mode and add unit test (flutter/plugin…
a744ddc 
…s#4695)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 28, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 28, 2022
@engine-flutter-autoroll
3cc18d9 [camera]refactor flash mode and add unit test (flutter/plugin…
f92a4b6 
…s#4695)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jmagman jmagman jmagman approved these changes

@stuartmorgan stuartmorgan Awaiting requested review from stuartmorgan

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

Assignees
No one assigned
Labels
p: camera platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hellohuanlin @fluttergithubbot @jmagman @stuartmorgan