Skip to content
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

Run on UiThread and few others #328

Merged
merged 2 commits into from Nov 17, 2021
Merged

Run on UiThread and few others #328

merged 2 commits into from Nov 17, 2021

Conversation

deakjahn
Copy link
Contributor

With reference to #272

So, a few words about the changes. First of all, sorry about some whitespace differences but, apparently, our IDEs use dartformat with slightly different settings.

The main difference is a new MethodResultWrapper class that wraps both the result and the channel. onMethodCall() now immediately saves this wrapped result-channel to a field and only uses that later to set both the result and to send back info on the channel. I did this in both Google and Amazon but I can't test the Amazon one.

Second, I included the plugin registration differences I suggested in the issue queue above.

Third, I did the modification suggested in one of the issues (I can't find it right now to link it, sorry) that initConnection, endConnection and consumeAllItems shouldn't be accessors. This is very much so, property accessors are not supposed to do work and have side effects, just return a value. I suggested now three new functions and marked the old ones deprecated, so you could safely leave it as is for some time, maybe until the next major version, if you accept the suggestion.

Fourth, EnumUtil.getValueString() is not really necessary, we have describeEnum() in the Flutter engine just for this purpose.

@hyochan
Copy link
Member

hyochan commented Nov 16, 2021

@deakjahn What a massive update. I'll try to go over this soon. Could you kindly check the CI failing?

@deakjahn
Copy link
Contributor Author

deakjahn commented Nov 16, 2021

The formatter exits with an error code 1 but without giving any meaningful error message. Hmmm. It runs for me locally. At any rate, this is the format you used earlier, so I'll upload with this new format now, there will be less difference compared to your current layout.

@deakjahn
Copy link
Contributor Author

deakjahn commented Nov 16, 2021

Yep. Flutter formatting defaults to 80-char line lengths which I, frankly, hate. I consider this setting fundamentally flawed with today's monitor sizes but I witnessed and even took part in many disputes with Munificient, the one man driving force behind dart (now flutter) format and he cannot be convinced otherwise. Now as it's reverted, the changes are less numerous, although in quite a few places I did change from setting a variable and immediately returning it to returning it in one step. This makes it look like a really massive change but the actual, meaningful changes are less than that.

Come to think of it, format had an --exit-if-changed or similar switch. That must be it. Because mine was different, it simply exited upon change, so the system simply refuses anything but its own format now. Nice. I'll have to keep that in mind because I'm sure as hell not to use 80-char lines in my own code... :-)

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #328 (54e8eb9) into master (6a79501) will increase coverage by 0.75%.
The diff coverage is 80.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
+ Coverage   45.22%   45.97%   +0.75%     
==========================================
  Files           3        3              
  Lines         429      435       +6     
==========================================
+ Hits          194      200       +6     
  Misses        235      235              
Impacted Files Coverage Δ
lib/utils.dart 64.70% <ø> (-3.72%) ⬇️
lib/flutter_inapp_purchase.dart 54.43% <80.43%> (+1.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a79501...54e8eb9. Read the comment docs.

@hyochan hyochan added the enhancement New feature or request label Nov 16, 2021
Copy link
Member

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

Looks great to me so far.

Let's dangerously release to production 🙏

@hyochan hyochan merged commit 229db8d into dooboolab-community:master Nov 17, 2021
@deakjahn
Copy link
Contributor Author

@hyochan I switched to the published 5.1.1 in my code and it seems to work OK. :-)

@mgonzalezc
Copy link
Contributor

Hello,

this PR makes the method getPurchaseHistory raise an IllegalArgumentException. Please, look at my comment in #339 where I mention the issue

@deakjahn would you mind having a look? I've tried to deep into your code but I am not sure if I will break something else.

Thank you

@deakjahn
Copy link
Contributor Author

deakjahn commented Feb 14, 2022

It was quite some time ago and I forgot what it was all about, now I can see. I don't get any error in my app but it might not use the very same services. Didn't the modification suggested by 0x-cell solve it? Because I can't seem to have the same error, I woudn't be able to reproduce it.

@deakjahn
Copy link
Contributor Author

@mgonzalezc I spoke too soon, I started receive something but very recently, only as of yesterday. Not the very same (null in line 59 of MethodResultWrapper but could very well related). Strange because I haven't updated that app in at least a month now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants