fixes and refactoring start

martinetd commented 2 years ago

first few commits are small fixes, probably OK. might want to push them as a separate PR to merge and stop worrying about it so the MainActivity.kt diff is clean when I start with it (soon)
last two commits start refactoring: for now I've just added plenty of types for SipMessage, plenty of todos, and a few questions

I've got plenty to talk about here but it's getting late and for now the only thing that matters is what you think of these types so we can iterate; I'd rather not to build too much on a house of cards.

If types are ok, next is the big questions I've left in comments regarding how to parse, the mix of BufferedReader and BytesArray for message body doesn't work well... But we don't care about the bodies yet, so assuming types are ok I could start using this in MainActivity.kt, or start digging at a network layer.

phhusson commented 2 years ago

That's all fine by me, I can probably merge it as-is since it doesn't seem you broke anything to me

martinetd commented 2 years ago

What does those two URLs look like?

P-Associated-Uri: <sip:+818012341234@ims.mnc051.mcc440.3gppnetwork.org>  
P-Associated-Uri: <sip:+818012341234@ims.imsnw.kddi.ne.jp>

It probably doesn't matter much, but my client then proceeds using the first one while your code originally overwrote it and used the later

martinetd commented 2 years ago

That's all fine by me, I can probably merge it as-is since it doesn't seem you broke anything to me

As for this (how to move forward): I'm fine either way.

It doesn't look like I'll need to do huge changes to the current code, so I think building incrementally instead of continuously rewriting history makes more sense -- I'm generally careful not to break things (e.g. introduce new interfaces then use them) so I agree it's probably fine to merge and will make further reviews easier so that's probably the way to go, but I can just keep piling up here as you prefer.

martinetd commented 2 years ago

One more question: I noticed I'm not using the same style (tab vs space etc) at all as in MainActivty.kt, and found some ktfmt tool.

It probably doesn't matter all that much at this point but should we get a run of that in before growing the code too much? (I don't use an IDE but that can be enforced at commit time or github action if we agree to it)

Unfortunately it looks like your style wasn't what they expect either, but it's somewhat close enough with the --kotlinlang-style switch

phhusson commented 2 years ago

I use android-studio so it's mostly using Android Studio's formatting Ok for ktfmt --kotlinglang-style.

It looks like using ktfmt in Android Studio requires a bit of manual setting, and there doesn't seem to be some generic file header that text editors could read to know which ktfmt setting to use?

martinetd commented 2 years ago

No idea, I'm running good ol' vim with little integration :/ As long as it's something I can run on git commit I don't really care what we use, or if we use nothing at all - I just like to have some automation to catch tabs when we're not using them, and since I'm not familiar with kotlin I wanted to have something confirm where standard line breaks happen etc (I tried using when () -> with a block and wondered what was appropriate for that when I looked for ktfmt)

android studio seems to support .editorconfig so perhaps just that'd be enough?

martinetd commented 2 years ago

I've:

added a SipReader class over BufferedInputStream (I didn't notice the difference between BufferedReader and BufferedInputStream that you were using in MainActivity.kt... I wasn't getting why you had to reimplement lines() but it all makes sense now :|)
That SipReader is a bit more specialized:
- it trims carriage returns like bufferedReader's
- it stops on empty line automatically, this isn't 100% needed but makes for something easier to use and we need empty line handling anyway for the next point:
- it peeks at next lines to handle continuations at this level, this is safe for SIP as we know there's more to read
switched SipMessage to use it
added editorconfig, that'll be good enough for now. I did pass ktfmt once just to see and comitted that for MainActivity, but I'd be happy to remove that commit, I don't really care as long as tab/spaces aren't mixed.

So now my SipMessages should be good enough for you as well (line continuations got there), I'll switch MainActivity to use it next and then will look at separating network out. (EDIT: next = my tomorrow)

martinetd commented 2 years ago

Sorry for the delay, took me a while to get an interface I somewhat like.

quick update:

I've moved away from data class for sip messages (response/request) as these don't allow constructor to actually do useful things e.g. enrich headers
I've stopped splitting parameters by default, it can be done when required with getParams()
I need to check how to split a program in multiple packages if that make sense, otherwise we can just use me.phh.sip everywhere...
I've finally started touching MainActivity and changed the very first message write, tested on my phone and it appears to work.
(last commit also sneaked in that I keep reading from the client socket instead of closing it, this fixes tcp for me -- I'll split that change out later and force push just that last commit)

I'll just update MainActivity to use the new SipRequest/SipResponse and that should fix most of the immediate problems you were reported the other day and hopefully not break much. When that's done I'll need to sit down and take time to think about network, MainActivity does quite a lot and we need to keep track of IPs/ports used in the messages themselves so a kludgy abstraction won't work. I'd still like to separate it and have callbacks though. Will keep you updated.

martinetd commented 2 years ago

ok that's about it for the first pass using new types -- the logic is mostly untouched so I don't think I broke anything, but tcp still doesn't work well for me so I didn't get anything past the register itself.

next would be breaking things out a step further, but I'm still undecided on how to abstract the network away.. I guess I'll just try something and see how it goes...

If you have time for a new review, now'd be a good time to take a fresh look at the type changes as it's evolved quite a bit since last week, but I hope I've kept it in a general direction you're comfortable with? Please tell me if there's something you'd see differently.

pihug12 commented 1 year ago

Sorry if it's not the right place to ask, but is there still some works done on this? Or should I follow this somewhere else?

Many thanks for this effort :slightly_smiling_face:

martinetd commented 1 year ago

hi @pihug12 -- I'm still working on it (a bit); the /e/ foundation is financing development so the "active" branches have moved to their gitlab which isn't public; and this branch has been more or less left as is since.

https://github.com/martinetd/android_ims has up-to-date-ish developments; but it now needs to be built in-tree because the IMS API isn't available to normal builds, and it's still nowhere to complete (the address for outgoing SMS isn't always correct and we have no idea where one could get it from, calls aren't implemented at all, the app isn't properly backgrounded...) If you can or know someone who can help on any of these issues we'd be happy to delegate some of it and explain what's needed to get started :)

phhusson / ims

fixes and refactoring start #1