[geolocator] Refactor the C++ code #387

JSUYA · 2022-05-16T09:12:38Z

Remove unused header
Change header's macro name
Fix wrong file name
Use typedefs of long commonly-used types
Cleanup error message

swift-kim · 2022-05-16T14:02:04Z

Tip) Applying https://github.com/flutter-tizen/flutter-tizen/wiki/Style-guide and https://github.com/flutter-tizen/flutter-tizen/wiki/Recommended-settings-for-VS-Code will help formatting your code.

JSUYA · 2022-05-17T01:46:17Z

@swift-kim Thank you for tip

and I have a question regarding some github action failures related to this PR.
Could you please check these?

Please let me know if there are any additional actions I need to take

[build test]
When I ran this command in my local environment, the build test became success.
./tools/tools_runner.sh build-examples --run-on-changed-packages --base-sha=$(git rev-parse HEAD^)
But the github action failed with this log.

[   +4 ms] Failed to download https://github.com/flutter-tizen/engine/releases/download/6ac2e23/tizen-common.zip. Ensure you 
have network connectivity and then try again.

[dart_analyze fail]
https://github.com/flutter-tizen/plugins/runs/6449786679?check_suite_focus=true
sqflite/example is fail

[integration test]
https://github.com/flutter-tizen/plugins/runs/6449786692?check_suite_focus=true

Cannot cache tizen artifacts used for integration-test.
Error: Process completed with exit code 1.

swift-kim · 2022-05-17T01:49:30Z

@JSUYA Please check my M Chat message. It could be a DNS issue that should be resolved by the GitHub team.

packages/geolocator/CHANGELOG.md

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

swift-kim · 2022-05-17T04:46:24Z

packages/geolocator/tizen/src/permission_manager.h

@@ -1,12 +1,11 @@
 // Copyright 2021 Samsung Electronics Co., Ltd. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be


#353

Our plan was to use the same implementation of PermissionManager for all plugins that need permissions (camera, image_picker, geolocator, and permission_handler). You can replace the files in this package with the files copied from image_picker_tizen. Please let me know if there's any issue.

I changed the PermissionManager by referencing another package.
I added ChangePermissionResulttoLocationPermission and ChangePermissionStatustoLocationPermission because we have to keep the enum order used by the geolocator.

You can change the values of the PermissionStatus enum as necessary (and modify the implementation of PermissionManager accordingly) without having to convert between the types. Please refer to #379. The PermissionManager class in the permission_handler_tizen package has been copied from image_picker_tizen, but has only a single return type PermissionStatus because its corresponding Dart type is already defined in the plugin's platform interface.

I changed the PermissionStatus enum of PermissionManager.

swift-kim · 2022-05-17T04:56:33Z

packages/geolocator/tizen/src/setting.cc

 #include <app_control.h>
 #include <package_manager.h>

 namespace {


The PackageName class in this file doesn't have to be a class. It will make more sense if the class is replaced with the static GetPackageName function in #379.

Also consider refactoring the AppControl class in the same way as we've done in #379 to better handle error results.

I changed to app_settings_manager.

Well, the original AppControl class seems to make more sense than individual methods in new AppSettingsManager because it can better represent the lifecycle of an app_control handle. Just error handling was missing there. In that way you will have two classes:

AppSettingsManager (has only public methods)

AppControl (statically defined in app_settings_manager.cc, used by AppSettingsManager)

I changed app_control_h to a local variable and deleted the private methods of AppSettingsManager.

Alright. I'm okay with either way but you might discuss with the author of the original code (@bbrto21) about the change just in case.

packages/geolocator/tizen/src/setting.cc

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

swift-kim · 2022-05-17T05:34:52Z

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

          OnCancelGeolocatorUpdates();
          return nullptr;
        });
    geolocator_updates_channel_->SetStreamHandler(std::move(handler));


Please consider extending the flutter::StreamHandler (FlStreamHandler) class to create a stream handler class as I've done for battery_plus_tizen and connectivity_plus_tizen.

I haven't applied the StreamHandler class yet. Because I still don't perfect understand both StreamHandler and LocationManager. If that's okay with you, I'd like to make this a separate PR.

In order to refactor the LocationManager class, you may refer to

Connection::StartListen() of connectivity_plus_tizen which returns false on immediate error and takes a result callback which returns a ConnectionType on completion (Note that the ConnectionType enum has a kError value to represent an error state.)

ServiceManager of permission_handler_tizen whose return type is ServiceStatus

I extended 'LocationStream' and 'ServiceStatusStream' from 'LocationManager' to StreamHandler.
And I changed Error handling of LocationManager from TizenResult to throw exception. (For minimize use of output parameters)

packages/geolocator/tizen/src/app_settings_manager.cc

swift-kim · 2022-05-18T05:50:00Z

packages/geolocator/tizen/src/setting.cc

 #include <app_control.h>
 #include <package_manager.h>

 namespace {


Well, the original AppControl class seems to make more sense than individual methods in new AppSettingsManager because it can better represent the lifecycle of an app_control handle. Just error handling was missing there. In that way you will have two classes:

AppSettingsManager (has only public methods)

AppControl (statically defined in app_settings_manager.cc, used by AppSettingsManager)

packages/geolocator/tizen/src/permission_manager.cc

JSUYA · 2022-05-20T02:44:32Z

oh.. I will upload a patch for fix this soon
https://github.com/flutter-tizen/plugins/runs/6518068677?check_suite_focus=true#step:5:49

packages/geolocator/tizen/src/app_settings_manager.cc

packages/geolocator/tizen/src/app_settings_manager.h

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

packages/geolocator/tizen/src/permission_manager.h

packages/geolocator/tizen/src/permission_manager.cc

swift-kim · 2022-05-24T01:59:12Z

packages/geolocator/tizen/src/permission_manager.cc

-      LOG_ERROR("Unknown ppm_request_result_e.");
-      on_failure(TizenResult(ret));
-      break;
+      throw PermissionManagerError("Failed to request permission.");


According to the original PermissionManager class, the method should return kDenied here (although the default case should never happen). Please keep the logic consistent if there's no other special reason.

Also I'm okay with using PermissionManagerError instead of the error value kError but still, using kError should slightly be better in terms of consistency. Please always feel free to ask me before making changes when you're in doubt.

I added a kError enum and modified it to return this. and removed PermissionManagerError class

packages/geolocator/tizen/src/location_manager.cc

packages/geolocator/tizen/src/location_manager.h

packages/geolocator/tizen/src/location_manager.cc

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

swift-kim · 2022-05-24T06:06:45Z

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

+          [result_ptr](LocationManagerError error) {
+            if (result_ptr) {
+              result_ptr->Error("Operation failed", error.GetErrorString());
+              delete result_ptr;
+            }
+          });
+    } catch (const LocationManagerError &error) {
+      if (result_ptr) {
+        result_ptr->Error("Operation failed", error.GetErrorString());
+        delete result_ptr;
+      }


Please use only one error handling mechanism at a time (either error callback or exception). You can remove the try-catch block here and modify GetCurrentPosition accordingly.

The error callback is the error handling of the callback function of location_manager_request_single_location called in GetCurrentPosition. As far as i know, since this callback is called asynchronously, it was not possible to throw an exception inside the callback.

There is a way to change GetCurrentPosition's return type to bool type and not use exception. (maybe the way you want)
I thought these three methods of LocationManager should be implemented in the same context: GetCurrentPosition, GetLastKnownPosition, IsLocationServiceEnabled.
(Reduce output arguments. Use Exceptions Rather Than Return Codes(maybe clean code?))
I don't think there is any particular problem with this way. Do you still think you should fix GetCurrentPosition to type bool and remove exception throwing? If you want to change it, I'll change it. Or please let me know if there is another way

Sorry for not being clear. What I meant by the above comment was just

@@ -93,7 +93,8 @@ void LocationManager::GetCurrentPosition( }, this); if (LOCATIONS_ERROR_NONE != ret) { - throw LocationManagerError(ret); + on_error_callback_(LocationManagerError(ret)); + on_error_callback_ = nullptr; } }

In this way, you don't need a try-catch. Thank you for your careful consideration.

I modified it according to your guide.

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

packages/geolocator/tizen/src/app_settings_manager.h

packages/geolocator/tizen/src/location_manager.h

- Remove unused header - Change header's macro name - Fix wrong file name - Use typedefs of long commonly-used types - Cleanup error message

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

swift-kim · 2022-05-26T09:29:27Z

packages/geolocator/tizen/src/location_manager.cc

+    LocationCallback on_location_callback,
+    LocationErrorCallback on_error_callback) {
+  on_location_callback_ = on_location_callback;


Callback arguments can be named like on_foo or foo_callback but you shouldn't mix the two.

Suggested change

LocationCallback on_location_callback,

LocationErrorCallback on_error_callback) {

on_location_callback_ = on_location_callback;

LocationCallback on_location_update,

LocationErrorCallback on_error) {

location_callback_ = on_location_update;

This is what I meant in #387 (comment). I don't think we need to rename LocationCallback to LocationUpdateCallback and location_callback_ to location_update_callback_.

You might also refer to https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#naming-rules-for-typedefs-and-function-variables although it's not specifically for C++ code.

@swift-kim
Thank you for detail review.
I have a question. about "the method that is called" in link.
According to the link, the location_callback_ you suggest should be handle_location_, shouldn't it?

And there are two cases of using LocationCallback. (stream, getCurrentPosition).
So I think this is appropriate. What do you think?

LocationCallback handle_location_update_; LocationCallback handle_location_; ... StartListenLocationUpdate(LocationCallback on_location_update) handle_location_update_ = on_location_update; ... GetCurrentPosition(LocationCallback on_location,LocationErrorCallback on_error handle_location_ = on_location;

location_callback_ should be handle_location_, shouldn't it?

No, handleFoo in the document refers to an explicit method or function like this:

static void HandleFoo(void* value) { // Handle the value. } SetFooHandler(HandleFoo);

What do you think?

Yes, that looks good but the data member names would be:

LocationCallback location_callback_; LocationCallback location_update_callback_;

Thank you for guide. I changed it

packages/geolocator/tizen/src/permission_manager.cc

packages/geolocator/tizen/src/location_manager.h

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

packages/geolocator/tizen/src/location_manager.cc

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc

swift-kim

Thank you very much!

JSUYA requested review from HakkyuKim, bbrto21 and swift-kim May 16, 2022 09:12

github-actions bot added the p: geolocator label May 16, 2022

JSUYA marked this pull request as draft May 16, 2022 09:18

github-actions bot added the needs-publishing The package should be published after merge label May 16, 2022

JSUYA marked this pull request as ready for review May 17, 2022 01:46

swift-kim reviewed May 17, 2022

View reviewed changes

swift-kim mentioned this pull request May 17, 2022

[ci] Change the release tag format flutter-tizen/engine#287

Merged

swift-kim reviewed May 18, 2022

View reviewed changes

packages/geolocator/tizen/src/permission_manager.cc Outdated Show resolved Hide resolved

JSUYA force-pushed the dev/refactor_geolocator branch from 89b42aa to d826bf6 Compare May 20, 2022 02:32

bbrto21 reviewed May 20, 2022

View reviewed changes

packages/geolocator/tizen/src/geolocator_tizen_plugin.cc Outdated Show resolved Hide resolved

JSUYA force-pushed the dev/refactor_geolocator branch from 313eff4 to 1754e7a Compare May 20, 2022 05:48

bbrto21 approved these changes May 23, 2022

View reviewed changes

swift-kim reviewed May 24, 2022

View reviewed changes

HakkyuKim mentioned this pull request May 25, 2022

Publish packages automatically after merge #394

Merged

9 tasks

JSUYA added 6 commits May 25, 2022 16:26

[geolocator] Refactor the C++ code

1e697e4

- Remove unused header - Change header's macro name - Fix wrong file name - Use typedefs of long commonly-used types - Cleanup error message

[geolocator] Clean up code foramt

6a97435

[geolocator] Update ChangeLog

7cddeb6

[geolocator] Fix code format

a08a644

[geolocator] Replace PermissionManager code

5fee8d5

[geolocator] Enhance app setting manager

74e215f

JSUYA added 7 commits May 25, 2022 16:26

[geolocator] Enhance log message

d20a31f

[geolocator] Change PermissionStatus enum

822e7e6

[geolocator] Refactoring AppSettingsManager

8de89fb

[geolocator] Refactor LocationManager

7996eaa

[geolocator] Fix typo

9e01be1

[geolocator] Fix permission manager

a7edb92

[geolocator] Fix MothodResult handling

6813108

JSUYA force-pushed the dev/refactor_geolocator branch from a4a1c7c to 9b4f99d Compare May 26, 2022 02:18

[geolocator] Fix code

ef7448e

JSUYA force-pushed the dev/refactor_geolocator branch from 9b4f99d to ef7448e Compare May 26, 2022 02:22

[geolocator] Fix build

dc67c40

swift-kim reviewed May 26, 2022

View reviewed changes

swift-kim reviewed May 27, 2022

View reviewed changes

[geolocator] Fix more code

b8444e3

JSUYA force-pushed the dev/refactor_geolocator branch from 7b4b12c to b8444e3 Compare May 27, 2022 06:05

swift-kim reviewed May 27, 2022

View reviewed changes

[geolocator] Fix stream error handling

50086e5

swift-kim approved these changes May 30, 2022

View reviewed changes

JSUYA merged commit a1fe17d into flutter-tizen:master May 31, 2022

JSUYA deleted the dev/refactor_geolocator branch May 31, 2022 02:24

swift-kim mentioned this pull request Jun 29, 2022

[flutter_tts] Refactor the C++ code #401

Merged

swift-kim mentioned this pull request Sep 29, 2022

[battery_plus] Refactor the C++ code #351

Merged

		@@ -1,12 +1,11 @@
		// Copyright 2021 Samsung Electronics Co., Ltd. All rights reserved.
		// Use of this source code is governed by a BSD-style license that can be

[geolocator] Refactor the C++ code #387

[geolocator] Refactor the C++ code #387

Uh oh!

Conversation

JSUYA commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-kim commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JSUYA commented May 17, 2022

Uh oh!

swift-kim commented May 17, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JSUYA commented May 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JSUYA commented May 16, 2022 •

edited

Loading

swift-kim commented May 16, 2022 •

edited

Loading