Implement camera switching #51

ikbendewilliam · 2021-12-07T16:20:36Z

Fixed #4

Feature/can switch camera

Xazin · 2021-12-23T16:47:15Z

fast_barcode_scanner/example/lib/scanner_screen.dart

@@ -15,6 +15,20 @@ class ScannerScreen extends StatefulWidget {

 class _ScannerScreenState extends State<ScannerScreen> {
  final _torchIconState = ValueNotifier(false);
+  var _canChangeCamera = false;


Good practice sake should declare type bool

@Xazin , actually the Dart style guide encourages the use of var/final for local variables instead of declaring types::
https://dart.dev/guides/language/effective-dart/design#dont-redundantly-type-annotate-initialized-local-variables

This cannot be final, and I cannot find the place in the effective dart guidelines where it says to prefer var over declaring it the known type bool. And it depends highly on how narrow the scope the variable is used in is. In my opinion this is not a narrow scope.

Internally we always use var, but as this is not our package, and I can't find any best practise references I've changed it to bool

This cannot be final, and I cannot find the place in the effective dart guidelines where it says to prefer var over declaring it the known type bool. And it depends highly on how narrow the scope the variable is used in is. In my opinion this is not a narrow scope.

@Xazin sorry that my comment wasn't clear. I mentioned both var and final. Yes, this can't be final since you need to mutate the value, but it should be var. The part of the dart style guide that mentions this best practice is here:

https://dart.dev/guides/language/effective-dart/design#do-type-annotate-fields-and-top-level-variables-if-the-type-isnt-obvious

In this case the initializer makes the type obvious; it's initialized to false so it's obviously a bool and typing it out doesn't add any clarity.

So I believe the dart guide here recommends that this be var rather than bool.

I agree, but they also recommend doing this mostly in smaller functions. When in doubt always declare type, they even mention this themselves.

Although the subject is a bit contradictory even from their own documentation, I do agree if we're talking about functions of 10 lines or less, it makes no sense to declare type, but this is a local variable with a scope of 50+ lines.

Whether it adds clarity to type the variable, I can also agree with you that it's not because it has super much value, because the naming of the variable now correctly reflects what type it is as well. But with a larger scope and 1 character difference, I don't see a good enough reason to omit the type in this scenario.

@Xazin I hope you don't mind a continued discussion on this. It seems like such a small thing; but since we're talking about enforcing a convention I'm more interested in making sure we get the convention right than about this particular line of code. As as a contributor to this package I want to make sure my own contributions meet the package's standards.

Ultimately, it's neither right nor wrong to add the type annotation in this location. I think it should be fine if we do either; though I personally I take a minimalistic approach and prefer to leave them off when the annotation doesn't add value.

From the style guide section I shared it specifically says this:

In some cases, though, the type is so obvious that writing it is pointless:

And then as you pointed out it also says it's ok to add them even if it's obvious:

When in doubt, add a type annotation.

And so the use of the annotations requires some judgement. In addition to considering the size of the function I think it is also useful to consider the complexity of the expression the produces the type. Complex expressions can make the type more difficult for the reader to infer. consider these examples:

complex expression

final firstList = <int>[1, 2, 3, 4]; final secondList = firstList .map((e) => e.toString()) .map((e) => e.length) .map((e) => e % 2 == 0 ? "even" : "odd") .toList();

In this first example you would have to mentally evaluate the expression before you know what type secondList is. This would probably be fine in a small function where the logical flow of the program is easy to evaluate and follow. But in a bigger scope a type annotation would be useful in this spot to save the mental energy. Furthermore, a type annotation is helpful to avoid an accidental List<dynamic> inference which is easy to do when mutating lists and the tools wouldn't necessarily help you see the issue.

This would be much better with generic types and type annotations:

final firstList = <int>[1, 2, 3, 4]; final List<String> secondList = firstList .map<String>((int e) => e.toString()) .map<int>((String e) => e.length) .map<String>((int e) => e % 2 == 0 ? "even" : "odd") .toList();

simple expression

var _canChangeCamera = false;

In this second example, taken from the code in question, false is a reserved primitive type that is completely obvious. Adding a type annotation to a simple and unambiguous expressions like this can be avoided.

In the end I think our opinion here is actually very close. You don't find a good enough reason to omit the type and I don't find a good enough reason to add the type. And so ultimately I think we should support either style; and in this case, leave this line how it is. A rule to always add or always omit isn't workable.

Xazin · 2021-12-23T16:48:18Z

fast_barcode_scanner/example/lib/scanner_screen.dart

+    _checkCamera();
+  }
+
+  Future<void> _checkCamera() async {


Naming sense here could be better, you're not checking the camera, you're checking if it's possible to change camera.

suggest: _validateCameraSwitching

Xazin · 2021-12-23T16:49:17Z

fast_barcode_scanner/example/lib/scanner_screen.dart

+  Future<void> _checkCamera() async {
+    final canChangeCamera = await CameraController.instance.canChangeCamera();
+    setState(() {
+      _canChangeCamera = canChangeCamera;


You can change this to

setState(() async { _canChangeCamera = await CameraController.instance.canChangeCamera(); });

No need for variable declaration.

Xazin · 2021-12-23T16:50:46Z

fast_barcode_scanner/ios/Classes/BarcodeReader.swift

+        do {
+            try setupCaptureDevice(arguments)
+        } catch {
+            throw error
+        }
+	}


Formatting in this file seems very off.

ikbendewilliam · 2021-12-29T11:11:23Z

@Xazin Can you review again? The comments have been fixed

Xazin · 2021-12-29T18:52:22Z

@Xazin Can you review again? The comments have been fixed

@jhoogstraat Is the only one who can merge it, and he also needs to review it.

Also I think you should change the branch you're merging into to develop !

jhoogstraat · 2021-12-29T23:21:11Z

Yeah, sorry for being to quiet. I am finalizing my master thesis, so after that I will be back working on this plugin.
Does this fix issues which are present in the develop branch?

Xazin · 2021-12-29T23:33:41Z

This is the missing functionality to switch between front and back camera, I am not sure how the develop branch looks, but this indeed does fix this part of the issue on main.

ikbendewilliam · 2021-12-30T07:41:20Z

No worries, that has priority of course!
This adds functionality in the current release v1.1.4 (main branch). I haven't tested it on the v2 (develop branch)

ikbendewilliam · 2022-02-07T10:02:44Z

Hi @jhoogstraat, I hope you're doing well.
In the near future we'd like to use this implementation in a new package, to publish that package this first needs to be merged. For now we use git dependencies 🙂.
This isn't priority as you're working on your master thesis, but can you give us an indication of when you would be able to check this? Or, if you prefer, you could give @Xazin merge rights (or someone else ofc). But that's entirely up to you.

jhoogstraat · 2022-02-07T12:59:41Z

Hi, actually I am back right now. I am currently looking through all the comments and changes!

ikbendewilliam · 2022-02-28T11:18:19Z

Any update on this?

jhoogstraat · 2022-03-01T17:20:40Z

I would very much like to merge your pr, but I am still hestitent as I am unsure how to incorporate these changes into v2 (develop branch).

Maybe with a bit of refactoring this approach can be used.

…ntinuous-detection-mode Enable pause & resume on continuous scanning mode in iOS

vanlooverenkoen · 2022-10-14T11:49:30Z

@jhoogstraat any idea when we can get this merged or when v2 will be done?

jhoogstraat · 2022-10-20T22:04:40Z

I think there is not much left for the release of v2. I'll do some tests and check with other issues/prs.

vanlooverenkoen · 2022-10-20T22:05:53Z

Thanks, we are looking forward to that so we can update our package and release them to pub.dev as well

vanlooverenkoen · 2023-04-11T11:49:09Z

@jhoogstraat any update on this?

ikbendewilliam · 2023-07-06T18:24:07Z

@jhoogstraat any more updates? 😁

jhoogstraat · 2023-07-26T05:56:32Z

Hey, I'll take at look at the code.
Is this ready to merge?

vanlooverenkoen · 2023-07-26T06:40:20Z

We are using this in production for 1,5 years already

ikbendewilliam · 2023-07-28T14:59:50Z

Yes it's ready to merge and indeed, we've been using it in different projects that are in production

vanlooverenkoen · 2023-08-16T08:54:05Z

@jhoogstraat any idea when this can be merged?

ikbendewilliam and others added 5 commits December 7, 2021 17:15

Implement camera switching for Android

762a559

Add method canChangeCamera

c42e65a

Switch camera on iOS

22d7372

Fix build issue

7e5e25e

Merge pull request #1 from ikbendewilliam/feature/can-switch-camera

dd764a8

Feature/can switch camera

ikbendewilliam changed the title ~~Implement camera switching for Android~~ Implement camera switching Dec 9, 2021

Xazin reviewed Dec 23, 2021

View reviewed changes

Fixed PR Comments

8f5c69a

This comment was marked as outdated.

Sign in to view

rwillemsandroid and others added 2 commits April 21, 2022 14:11

Enable pause & resume on continuous scanning mode in iOS

4ff1eee

Merge pull request #2 from rwillemsandroid/feature/pause-resume-in-co…

4aca0cd

…ntinuous-detection-mode Enable pause & resume on continuous scanning mode in iOS

vanlooverenkoen mentioned this pull request Oct 14, 2022

Not found FastBarcodeScanner() icapps/combined_barcode_scanner#8

Closed

Update kotlin version

c734a89

Flutter update

dfbd6ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement camera switching #51

Implement camera switching #51

ikbendewilliam commented Dec 7, 2021

Xazin Dec 23, 2021

dustin-graham Jan 8, 2022

Xazin Jan 8, 2022 •

edited

Loading

ikbendewilliam Jan 14, 2022

dustin-graham Feb 7, 2022

Xazin Feb 7, 2022

dustin-graham Feb 10, 2022

Xazin Dec 23, 2021

dustin-graham Jan 8, 2022

Xazin Dec 23, 2021 •

edited

Loading

Xazin Dec 23, 2021

ikbendewilliam commented Dec 29, 2021

Xazin commented Dec 29, 2021 •

edited

Loading

jhoogstraat commented Dec 29, 2021

Xazin commented Dec 29, 2021

ikbendewilliam commented Dec 30, 2021

ikbendewilliam commented Feb 7, 2022

jhoogstraat commented Feb 7, 2022

ikbendewilliam commented Feb 28, 2022

jhoogstraat commented Mar 1, 2022

This comment was marked as outdated.

vanlooverenkoen commented Oct 14, 2022

jhoogstraat commented Oct 20, 2022

vanlooverenkoen commented Oct 20, 2022

vanlooverenkoen commented Apr 11, 2023

ikbendewilliam commented Jul 6, 2023

jhoogstraat commented Jul 26, 2023

vanlooverenkoen commented Jul 26, 2023

ikbendewilliam commented Jul 28, 2023

vanlooverenkoen commented Aug 16, 2023

Implement camera switching #51

Are you sure you want to change the base?

Implement camera switching #51

Conversation

ikbendewilliam commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazin Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

complex expression

simple expression

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazin Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikbendewilliam commented Dec 29, 2021

Xazin commented Dec 29, 2021 • edited Loading

jhoogstraat commented Dec 29, 2021

Xazin commented Dec 29, 2021

ikbendewilliam commented Dec 30, 2021

ikbendewilliam commented Feb 7, 2022

jhoogstraat commented Feb 7, 2022

ikbendewilliam commented Feb 28, 2022

jhoogstraat commented Mar 1, 2022

This comment was marked as outdated.

vanlooverenkoen commented Oct 14, 2022

jhoogstraat commented Oct 20, 2022

vanlooverenkoen commented Oct 20, 2022

vanlooverenkoen commented Apr 11, 2023

ikbendewilliam commented Jul 6, 2023

jhoogstraat commented Jul 26, 2023

vanlooverenkoen commented Jul 26, 2023

ikbendewilliam commented Jul 28, 2023

vanlooverenkoen commented Aug 16, 2023

Xazin Jan 8, 2022 •

edited

Loading

Xazin Dec 23, 2021 •

edited

Loading

Xazin commented Dec 29, 2021 •

edited

Loading