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
[permission_handler] Support Tizen 4.0 #139
Conversation
for (size_t i = 0; i < permissions_to_request.size(); i++) { | ||
const char* permission = permissions_to_request[i]; | ||
p.is_done = false; | ||
result = ppm_request_permission( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about extracting this callback method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... this is close to my personal preference, I like to avoid adding unnecessary functions to class members or exposing them in headers and I generally prefer lambda functions unless they are used forever elsewhere.
However, I respect your opinion and it is possible :) Should I change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😃 this is also my preference, not a critical. I just found this lambda function too long to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this PR
|
||
// Wait until ppm_request_permission is done; | ||
while (!p.is_done) { | ||
ecore_main_loop_iterate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we said in our conversation, I don't think this is strange. I'm not sure if timeout
is necessary here. I think we should listen to other people's opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have other reasonable ideas, please share it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems only God can use this function according to its documentation. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a funny document! 😆
I'm not God, but there's nothing much the user can do when show pop-up for a permission, so I thought it was OK.
It's a little different story... |
I couldn't find any information related to the expiration of |
* Use ppm_request_permission instead of ppm_request_permissions * Now the requests are executed sequentially, one by one Signed-off-by: Boram Bae <boram21.bae@samsung.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your working!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me.
const char** privileges, size_t privileges_count, void* user_data) { | ||
if (!user_data) { | ||
LOG_ERROR("Invalid user data"); | ||
void OnRequestPermissionsRespon(ppm_call_cause_e cause, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any special reason why you shortened the name of this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please refer to the conversation containing this comment?
When I changed this lambda function back to a named function, I made it a local function and excluded it from the class namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnRequestPermissionsResponse ->OnRequestPermissionsRespon
Oh!? Are you talking about this? My mistake! I'll fix it later when I get to work.
* Change a long lambda to local function to improve readability * Apply naming convention * Use enum class instead of enum * Use constexpr rather than macro Signed-off-by: Boram Bae <boram21.bae@samsung.com>
Signed-off-by: Boram Bae boram21.bae@samsung.com