Adamcake / Bolt

An alternative launcher for your favourite MMO
GNU Affero General Public License v3.0
162 stars 23 forks source link

UI & UX Changes #30

Closed smithcol11 closed 5 months ago

smithcol11 commented 6 months ago

Hello, I have been a big fan of the project and wanted to give back in some way! I am not sure if there have been plans to update the UI/UX, but I thought I would throw something together to see what you and others thought. As far as I can tell I haven't introduced any problems or regressions in functionality.

Key Changes

Screenshots of Updated UI/UX

Screenshot_20240324_200509 Screenshot_20240324_200613 Screenshot_20240324_200624 Screenshot_20240324_200712

Thoughts

I am very keen and open to hearing your feedback. I am aware that this is likely just a draft or something you will scrap entirely. Please reach out with questions and suggestions!

Cheers

Adamcake commented 6 months ago

The second drop-down seems buggy now. I have two game accounts on my Jagex account but when I log in it only shows the first one. If I log in again, the top drop-down has two entries in it as expected, but after switching between them the bottom one either has 1 or 0 entries in it, when it should always have 2.

Adamcake commented 6 months ago

Well, to be honest I was hoping someone would do this so I wouldn't have to do it myself, so thank you. I have a few points of feedback on the PR itself.

smithcol11 commented 6 months ago

The second drop-down seems buggy now. I have two game accounts on my Jagex account but when I log in it only shows the first one. If I log in again, the top drop-down has two entries in it as expected, but after switching between them the bottom one either has 1 or 0 entries in it, when it should always have 2.

I was worried I introduced some kind of bug like this. I only have one account with one character so my testing was pretty limited. I will look into this and expand my testing!

smithcol11 commented 6 months ago

Glad to hear these changes are welcome! Regarding your feedback in order:

I appreciate the comments and feedback! I'll fix things we discussed above. Let me know what you think about adding a front end framework.

Thanks for you time! Cheers

Adamcake commented 6 months ago

React or similar would be nice if you can add it. It can make the build system quite bloated but the generated code in release mode is really small in my experience, so that's fine with me. There's also another issue, that anything involving npm is a massive headache to use in build systems such as Flatpak. I think the best solution to this is to commit packed sources to the repo and include an optional build step explaining how to repack them if people want to do that. I saw you've already committed tailwind's output.css.

On the other hand if you think React would make the page slower to load or interact then I trust your judgement, you seem to know what you're talking about. It seems like it would reduce development overhead a lot though, it's quite a chore to add features to right now as I'm sure you've noticed.

Adamcake commented 6 months ago

The icons are FOSS from FontAwesome. They use a CC license, as mentioned here on their site.

That's fine, thanks, just checking

smithcol11 commented 6 months ago

Agreed that a front end framework would make development a lot easier! just need to figure out some nuances like you mentioned. Yeah, committing the tailwind output was to allow people to just use it or repack it.

I should clarify that I am a fan of using frameworks but am by no means an expert. I think their performance shouldn't be an issue. Companies like Netflix and Facebook rely on React for their apps, and they run great. Lol yeah good point, it was a bit of a chore!

I'll experiment with adding a framework and converting the html/js to using it. Then, report my findings and/or commit what I have working. Thanks for the feedback.

TormStorm commented 6 months ago

I really like the overall design and aesthetics, i just think it looks a bit weird that the buttons to launch the different clients are in the big box in the top right. I would suggest moving them below the account dropdowns in the side panel and use the big box for something like updates to Bolt. I also prefer the side panel being on the right side, but that is just my personal preference. image

smithcol11 commented 6 months ago

@TormStorm thanks for the feedback! I agree that the UI in its current state looks a little off. I spent a lot of time moving things around and have yet to be totally happy with it. You might be right, maybe I should take some design choices from the official launcher, which implements a few things you mentioned. I will post new screenshots as I have them to see what people think.

TormStorm commented 6 months ago

I was working on something similar a while back, so i understand that it can be difficult to figure out where the different elements should be. I don't think taking some design choices from the official client would be a bad thing, it might also make it a bit easier for people who are used to the official client. For reference here is an image of what i was working on, although i think yours looks much cleaner. image

smithcol11 commented 6 months ago

Ooo yeah I like where you were going with that a lot. I think as I try to construct the interface using a framework I will try to move things around to mimic yours and the official launcher a bit more. I appreciate the inspiration!

smithcol11 commented 6 months ago

Hey, just wanted to give an update with what I have been up to regarding UX changes and introducing a framework!

UX

Considering I was going to be taking the UI apart, I decided to do a redesign that I think is a lot better. Screenshot_20240327_131526 As you can see it takes a lot of inspiration from the official launcher and comments left by you both.

Framework

I did a bit of research before deciding on an implementation.

Here is what the file structure looks like, in a new folder called 'app', which would replace 'html':

Screenshot_20240327_131550

Next Task

The next thing I am working on is moving the 'launcher.js' code into respective components. This is going to take more of my time to implement I think. Reasons being I did the good old foot gun technique by introducing TS and having loads of errors in the original JS file; and relocating / refactoring the code will require me to understand it a lot more to ensure I move things where they belong while not introducing bugs.

Thanks for reading my update. Let me know what you all think! I look forward to your comments, suggestions, and questions.

Cheers

Adamcake commented 6 months ago

Thanks, I appreciate you giving updates. If you're taking launcher.js apart then it'll be important to know that getting a login_provider in your credentials is not possible anymore, this was for non-jagex accounts which can no longer be used via the launcher. So you might like to save yourself some work by cutting out anything to do with login_provider.

smithcol11 commented 6 months ago

Yeah of course! Good to know, I will look to removing anything using login_provider. Thanks for the tip.

smithcol11 commented 6 months ago

Hello again, hope the weekend is going well! I wanted to give another update and push my changes for others to look at.

The frontend has been moved into app, using Svelte, TypeScript, and Tailwind; things seem to be working for the most part. I have been trying to do things to break it and have fixed everything I found up till now.

Some key updates and additions since the last update

Current tasks

Overall, I am happy with the progress I have made. I look forward to what you all think! Feedback, comments, concerns, and questions are appreciated. Let me know any bugs you find and I will set out to fixing them asap.

Thanks

Adamcake commented 5 months ago

This looks good so far, are you almost done with it?

Not sure if you already knew about this, but you've introduced a bug caused by launching RS3: you always set hash to an empty string in the URL params, which is causing a couple of different issues, most notably one on the C++ side which is preventing it from writing any URL params to launcher.html other than rs3_linux_installed_hash. https://github.com/smithcol11/Bolt/blob/ux/gui-changes-add-tailwindcss/app/src/functions.ts#L457-L460 I think this was probably a copy-paste error? Either way, I would prefer you don't send a given URL param at all instead of sending an empty one. jx_access_token and jx_refresh_token aren't relevant anymore as they were for non-jagex accounts.

Another (much less important) note: if a game account has just been created and no display name is set yet (setting display name is a part of tutorial island I think?), then displayName won't be present at all in the JSON response. I don't think your UI is handling this case since it says "undefined" in the drop-down menu for unnamed accounts. It should say something more helpful I think.

smithcol11 commented 5 months ago

Yeah it is very nearly done. Just been typing up loose ends here and there. Thanks for finding some bugs / undefined behavior! I was hoping you would find something I was overlooking. I will fix both of those things you mentioned today.

smithcol11 commented 5 months ago

I fixed those issues and others that I had as well. I think I would call it "done" at this point. Let me know other bugs or suggestions you find and I will take care of them. I appreciate the feedback.

smithcol11 commented 5 months ago

Wanted to mention, I changed the package manager to bun instead of npm. Checkout their website, it is a cool project: https://bun.sh/docs This changes the lock file. So instead of a package-lock.json, there is now a bun.lockb. I updated the README files regarding this change. Installing it is simple with npm.

Adamcake commented 5 months ago

https://github.com/Adamcake/Bolt/blob/5f5d83b4202deac7d78678416823a46ab41a7680/app/src/lib/TopBar.svelte#L47 Don't put the company name in the repo please.

Adamcake commented 5 months ago

Wanted to mention, I changed the package manager to bun instead of npm.

Yeah I know about bun. It certainly runs better and has a nice build system (zig.) There are a few things I don't like about it but none of them are relevant here.

Adamcake commented 5 months ago

Okay, last few issues:

Beyond that I agree, this is about ready to go.

smithcol11 commented 5 months ago

Thanks for finding some more bugs! It is funny how the order in which I Implemented things meant I never saw that undefined time one.

I just pushed a couple changes that fix everything you mentioned.

Let me know if you have anything else regarding style/code you want me to change.

Adamcake commented 5 months ago

Ah, just found another problem, and I really should've tested this sooner: logins expire after 30 minutes.

Okay, so, when a set of oauth credentials is granted by the login server, it has an "expiry" field which usually indicates a time 30 minutes after it was granted. If that has expired, you won't be able to use the credentials anymore for things like the displayName endpoint, but you can still use the refresh_token to renew them. checkRenewCreds renews them if they're expired.

I've just checked. 28 minutes after being granted, works fine. 32 minutes after being granted, I get a 401 from displayName. Which either means checkRenewCreds isn't being called or the new credentials aren't being saved to the file. (Or perhaps checkRenewCreds has been changed to operate by value instead of by reference?)

smithcol11 commented 5 months ago

Ah, just found another problem, and I really should've tested this sooner: logins expire after 30 minutes.

Yeah, I did notice weird behavior with the login expiring. I chalked it up to the dev environment but guess I shouldn't have assumed that. I will look into fixing it right now.

Adamcake commented 5 months ago

Well I think there is a separate issue where the hot-reload will re-use an old version of the credentials file. That one's my fault, not yours - you might want to turn off hot-reloading while you're investigating.

smithcol11 commented 5 months ago

I'm not having too much luck getting passed the xml.send request. I always get this 400 response:

{
    "error": "invalid_grant",
    "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client."
}

Leads me to assume something is wrong with the token. I am not super familiar with refresh tokens, but I am looking into it. Any ideas?

Adamcake commented 5 months ago

Just pushed a fix to master for the hot-reload issue if that's any help.

That's the most generic error message unfortunately. Can you show what request you're sending, with the sensitive parts censored out?

smithcol11 commented 5 months ago

Okay I merged your change in. This has to do with the checkRenewCreds request.

request url: https://account.jagex.com/oauth2/token

payload: {
  grant_type: refresh_token
  client_id: com_jagex_auth_desktop_launcher
  refresh_token: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
}

Yeah people online say finding the cause of this error can be a bit of a pain haha. I will do some more testing.

Adamcake commented 5 months ago

For me, taking the refresh_token from my creds file and putting into this request, I got a 200 response with some new credentials: curl https://account.jagex.com/oauth2/token -X POST -d "client_id=com_jagex_auth_desktop_launcher&grant_type=refresh_token&refresh_token=95WZ3aGBirQh[.....]" ... but when I took the refresh_token from the response and tried to do the same thing again, I got the same error as you.

Adamcake commented 5 months ago

Nevermind, I think I made a copy-paste error last time. It works every time now.

So what are you doing differently from that?

smithcol11 commented 5 months ago

I think my problem had to do with these lines here in checkRenewCreds. These are your original lines.

const c = parseCredentials(xml.response);
if (c) {
  Object.assign(creds, c);
  credentialsAreDirty = true;
  resolve(null);
}

Javascript does like weird pass by reference where you can modify what is inside an object but by reassigning you don't effect the original copy. 'creds' comes in as a parameter. I changed these lines here and I don't think I was assigning the fields properly due to a 'find and replace' bug.

I am testing now, gonna let my login expire and see what happens.

smithcol11 commented 5 months ago

Okay I think I've got it working. I testing by forcing my login to send a refresh on every boot and it was working. Thanks for the help with that!

Adamcake commented 5 months ago

Yep I can't find any more issues with this. Thanks for your hard work.

smithcol11 commented 5 months ago

Awesome! Great to have it merged in, thanks for the help and feedback throughout. Let me know if you need anything else.

Adamcake commented 5 months ago

@smithcol11 I just found an issue in your PR by coincidence while testing something else. If you have multiple game accounts under one jagex account, the drop-down menu visually seems like it remembers which account you had selected, but if you click the play button it will always launch the first listed account unless you explicitly change it to a different one. Do you know why that would be the case?

smithcol11 commented 5 months ago

Yeah I have an idea; I keep track of the currently selected account, character, game, etc. I must not be setting it when it loads up preferences for that. I will look into it and open a PR soon, should be an easy fix I imagine.

Adamcake commented 5 months ago

Okay thanks. From what I've learned about Svelte so far I think this menu could be done with fewer variables than this. It says a Select's "value" attribute is the same as the "value" of the currently selected Option, and an Option's "value" can be anything, not just a string. So, if you use the {#each} block to set each Option's "value" to a Character object, and on the Select tag you set bind:value={someVariable}, then someVariable would update according to which Character is selected.

But that's just how I'd look at it. I'll leave it up to you since it's your PR.