search

farooqkz / chooj

Matrix chat app for KaiOS feature phones supporting voice calls
74 stars 13 forks source link

Login fix and improvements #95

Closed pinusc closed 9 months ago

pinusc commented 10 months ago

For now, this PR fixes #89, including updating the well-known information if supplied by the client. I opted to insist on the "well_known" abstraction even when an actual .well_known file or property are not returned by the server.

However, I only fixed the normal login flow. There seems to be a lot of duplicated code in LoginWithQR.tsx, and I have not touched the QR login yet.

I would like to refactor the code in Login.tsx and LoginWithQR.tsx by decoupling the actual login flow and logic from the presentation; something like a LoginHelper class to hold the logic, and then passing it to Login.tsx and LoginWithQR.tsx so that they use the same code underlying.

DRY and all; this will be especially important as more login flows are added, both to be typed manually and scanned with the QR code.

pinusc commented 10 months ago

I have now moved all the important login flow in LoginHandler.ts.
Sadly, a little bit of logic is still left for Login and LoginWithQR, because they do things in a different order - the first queries the server and asks for login flows first, and then asks the user for a password; the second does everything at once.

Depending on future implementation of login flows, this might still end up producing a lot of duplicated complexity, so maybe a more sophisticated implementation would be better. At this stage, however, I am not familiar with the other flows and so I can't plan particularly well around them.

This PR is now ready to merge.

Note: I rewrote most of the code using Promise to async/await, which is just syntactic sugar around promises. This enormously improves readability and makes it so much easier to reason with, and handle errors correctly; plus, since we are transpiling anyway, it works on any old version of Javascript with no issues, including KaiOS. For me, using async/await is a no brainer, but if @farooqkz has a reason to avoid it and stick with Promises, I guess I could rewrite it...

farooqkz commented 10 months ago

@pinusc Hello there and great work! I will review it soon and merge it God willing. Then we can move to codeberg.

farooqkz commented 9 months ago

@pinusc The review is done. Thank you so much for your contribution. I can merge it. Only you have to edit the files to follow JS naming schemes such as myName rather than my_name. Also after open parenthesis, there should be no space. Like (!s rather than ( !s. Then we can merge it.

farooqkz commented 9 months ago

Since it is taking too much time, I'm gonna merge this and fix the namings myself.