Skip to content

[audioplayers] Refactor the C++ code #357

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

Merged
merged 16 commits into from
May 12, 2022

Conversation

HakkyuKim
Copy link
Contributor

@HakkyuKim HakkyuKim commented Apr 14, 2022

Part of the code refactoring project.

Summary

  • Make internal linkages with unnamed namespace.
  • Use const instead of #define for constants.
  • Relocate constructor logic to RegisterWithRegistrar.
  • Use helper function GetValueFromEncodableMap.
  • Use enum class instead of enum.
  • Some naming changes.
  • Remove unnecessary logging.
  • Initialize private variables inline rather than in constructor.
  • Move HandleResult from AudioPlayer's memeber to internal free function.

@HakkyuKim
Copy link
Contributor Author

HakkyuKim commented Apr 14, 2022

@swift-kim @bbrto21
I'm not sure how I should handle errors. Google C++ style guide doesn't use exceptions (though this was a choice made due to specific situation at Google).

For functions that doesn't return any values but may throws exceptions, the code can be changed like this:

void Func(){
  ret = tizen_native_api();
  if(ret != ERROR_NONE) throw Exception();
}

// To:
bool Func(){
  ret = tizen_native_api();
  if(ret != ERROR_NONE) {
    // Detailed error can be checked by `last_error_`.
    last_error_ = ret;
    return false;
  }
  return true;
}

But what about functions that return a value? For example GetPlayerState? How does the caller know if the return value is valid or not? Should I wrap player_state_e with another enum that has an error entry like ConnectionType? Or should I rely on the caller to check last_error_ after calling a function that can fail? (If the calling member function is public, caller would check last_error_ with a getter function GetLastError()).

Out parameters may also be used but is it common in C++? I know it's commonly used in C but I haven't seen much in C++.

Any suggestions?

@swift-kim
Copy link
Member

swift-kim commented Apr 14, 2022

@HakkyuKim Although it's Google's internal convention to not use exceptions in their code, it would be good to keep that convention in our code because we also depend on their code. However, it's up to your decision.

If you decide not to use exceptions, there are several options. One is the way I used in my previous PRs, that is, using a result type that can represent an error state. The type can be either an enum class (has a kError value), pointer (nullptr on error), boolean (false on error), or a signed integer (negative value on error). For example, if the result type of the original native API is const char *, a function that wraps the API may look like

bool func(std::string *out_value) {
  char* result;
  int ret = tizen_native_api(&result);
  if (not success) {
    last_error_ = ret;
    return false;
  }
  *out_value = std::string(result);
  return true;
}

However, if you think repeating last_error_ = ret; throughout your code makes the code less readable and easy to break, you can define some explicit type like geolocator's TizenResult.

Another solution is to make the function take an error callback as an argument. You can use this approach if it makes most sense to you.

typedef std::function<void(int)> ErrorCallback;

void func(ErrorCallback on_error) {
  ...
  if (error) {
    on_error(ret);
  }
}

func([&](int ret) -> void {
  result_->Error(std::to_string(ret), get_error_message(ret));
});

@bbrto21
Copy link
Contributor

bbrto21 commented Apr 14, 2022

I'm not sure how I should handle errors. Google C++ style guide doesn't use exceptions (though this was a choice made due to specific situation at Google).

As far as I know, flutter-tizen is not using an explicit compile option(-fno-exceptions) to prevent the use of exceptions when building app.
so I think this is not a strong rule for plugin unless you do not propagate the exception outside of the plug-in library.
(I've actually already used exception depending on just my mood 😆 )

@HakkyuKim
Copy link
Contributor Author

@swift-kim @bbrto21
I've decided to leave exception mechanism for the moment. The code becomes messy when passing error handlers or returning error values for nested calls, and the current implementation of AudioPlayer internally call it's other public or private methods. I'll try this again in another PR.

@wanchao-xu
I'm trying to move calling Ecore related APIs to AudioPlayer, is there a reason why audio.onCurrentPosition events are checked by iterating all living AudioPlayer in UpdatePosition?(

while (iter != plugin->audio_players_.end()) {
)

@HakkyuKim HakkyuKim marked this pull request as ready for review April 28, 2022 07:01
@HakkyuKim HakkyuKim force-pushed the refactor-audioplayers branch from c6bef8c to 121e056 Compare April 29, 2022 03:41
@HakkyuKim
Copy link
Contributor Author

@swift-kim
Thanks for the detailed review, I've applied suggested changes and some others that would make the code more readable with the new changes.

@HakkyuKim HakkyuKim force-pushed the refactor-audioplayers branch 2 times, most recently from 2ec57ca to 2e15bbf Compare May 4, 2022 02:56
@HakkyuKim
Copy link
Contributor Author

HakkyuKim commented May 4, 2022

Hmmm don't know what's causing integration test to fail for audioplayers, same failure happens in master branch so it's not related to this PR change. This PR doesn't have the changes in #360 so it's not related to that either. I'll check.

@swift-kim
Copy link
Member

Hmmm don't know what's causing integration test to fail for audioplayers

The GitHub connection in the company is extremely slow today.

Downloading tizen-arm-debug tools...                             1,202ms
Downloading tizen-arm-profile tools...                           1,101ms
Downloading tizen-arm-release tools...                            111.4s
Downloading tizen-arm64-debug tools...                           2,550ms
Downloading tizen-arm64-profile tools...                         1,221ms
Downloading tizen-arm64-release tools...                          173.7s
Downloading tizen-arm-profile/linux-x64 tools...                 1,707ms
Downloading tizen-arm-release/linux-x64 tools...                   797ms
Downloading tizen-arm64-profile/linux-x64 tools...                 40.1s
Downloading tizen-arm64-release/linux-x64 tools...                 925ms

You may need to set a larger timeout value, or the artifacts should be downloaded in advance before the test is run (maybe using the flutter-tizen precache --tizen command).

@HakkyuKim HakkyuKim force-pushed the refactor-audioplayers branch from 0b8e0fb to fc31a7c Compare May 6, 2022 03:50
HakkyuKim added 12 commits May 6, 2022 12:52
- Make internal linkages with unnamed namespace.
- Use `const` instead of `#define` for constants.
- Relocate constructor logic to RegisterWithRegistrar.
- Use helper function `GetValueFromEncodableMap`.
- Use `enum class` instead of `enum`.
- Some naming changes.
- Remove unnecessary logging.
- Initialize private variables inline rather than in constructor.
- Move `HandleResult` from AudioPlayer's memeber to internal.
Also:
- Avoid c-style cast syntax.
- Declare local variable just before use.
- Remove unnecessary LOG_ERRORs.
- Rename method OnErrorOccurred -> OnError.
- Add newline at the end.
Also:
- Remove unused header cassert.
- Explicitly add used headers.
- Add return statements appropriately.
- Minor cleanups.
- Rename callback function: StartPlayingListener ->
  UpdatePositionListener.
- Call result->Success() in every if block to
  clarify execution path.
- Remove unused log.h include.
- Use ouput parameters for methods that return values.
- Use exceptions only in HandleMethodCall.
@HakkyuKim
Copy link
Contributor Author

@flutter-tizen/maintainers
Removed exception throws completely in 212d2b9, I'm fine either way but want to choose the one that others are comfortable with.

@HakkyuKim HakkyuKim force-pushed the refactor-audioplayers branch from 4e45da3 to 3046392 Compare May 9, 2022 01:14
@HakkyuKim HakkyuKim force-pushed the refactor-audioplayers branch from 3046392 to c8a3403 Compare May 9, 2022 05:44
@HakkyuKim
Copy link
Contributor Author

After discussing with @swift-kim, we've decided reverting back to using exceptions.

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

Looks nice! Thank you for the hard work.

Please do some testing on TV device/emulator before releasing.

Copy link
Contributor

@bbrto21 bbrto21 left a comment

Choose a reason for hiding this comment

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

Thank you for hard work.

@HakkyuKim
Copy link
Contributor Author

HakkyuKim commented May 11, 2022

There are some issues with audioplayers when running on TV (Not new ones from this PR, the problems existed in previous code), the main cause is calling player_destroy() in the OnPlayCompletedcallback function (it makes the player module crash silently). I'll leave this in the issues tab and fix it together when upgrading audioplayers to 1.0.0.

@HakkyuKim HakkyuKim force-pushed the refactor-audioplayers branch from ecfb3f6 to f121745 Compare May 11, 2022 02:29
@HakkyuKim HakkyuKim merged commit 6707534 into flutter-tizen:master May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants