victordiaz / PHONK

PHONK is a coding playground for new and old Android devices
https://phonk.app
GNU General Public License v3.0
457 stars 27 forks source link

Minor changes #133

Closed notEvil closed 1 year ago

notEvil commented 1 year ago

Hi,

those are just minor things that would pollute the list of issues and aren't worth individual PRs imo. If you think otherwise, let me know.

The current code ignores this body and params aren't populated as a consequence.

See https://github.com/victordiaz/PHONK/issues/131 Should receive a proper fix (using interface/base class) in the future

There is PApp.runOnUiThread for activities but it doesn't work for services. This works for both

In case you want to restrict the server to a subnet, like 127.0.0.1

The current code would try to release if called with false twice. This works with any call sequence

I've tested these changes for a week while developing an app and had no troubles :)

notEvil commented 1 year ago

Alright, just pushed a few more

I'm not familiar with Android development, so you should check if this is reasonable. At least my Android Studio compiles with this change and the Design view looks no different.

Those fixes some pain points when starting from scratch.

In the meantime I realized that the overload of createHttpServer doesn't align with the 1 argument version (port shifts to the right). Should I change this?

victordiaz commented 1 year ago

Just to let you know, I started this project long ago, so it has lots of suboptimal code decisions. Also I think the javascript world changed a lot, so the API does not feel very modern :)

I really appreciate every PR and comment. It makes me willing to keep working on it :)

notEvil commented 1 year ago
  • It makes sense what you say about the ip, port order. But also I think it looks more natural also to say ip, port. I let you pick whatever you think is nice.

Ok, then I won't change it. I've seen some overloads where this happens already and I personally prefer the "natural" sequence. If only there were keyword arguments ... xd

Just to let you know, I started this project long ago, so it has lots of suboptimal code decisions. Also I think the javascript world changed a lot, so the API does not feel very modern :)

True, but it otherwise works very well, even on my phone (~2017, community rom, android 11). So it passed the test of time imo.

I really appreciate every PR and comment. It makes me willing to keep working on it :)

Thats great, happy to contribute :)

victordiaz commented 1 year ago

I will merge this PR then