karaeren / loa-details

GNU General Public License v3.0
59 stars 14 forks source link

Re-add support for uploads and rewrite of application settings handling #30

Closed guy0090 closed 2 years ago

guy0090 commented 2 years ago

Depends on https://github.com/karaeren/loa-details-log-parser/pull/6

This PR aims to add basic support for uploading to any instance of LostArkLogs. A user must be authenticated to upload a log, i.e.: they will need to make an account on the instance and copy-paste their API key.

This version of my implementation will be somewhat inconsistent due to the way I am attempting to detect boss NPCs:

This is necessary because LostArkLogs only supports uploads for certain encounters (raids and guardians, more will be added soon).

I'll be working on a follow-up PR when I have the time and make it more consistent. If you'd like to wait to merge until that's ready, reject this PR.

Additionally, it includes a rewrite of the way app settings are handled. This includes switching to using a schema, which isn't all that different from what was done before, just that it's the method advertised on the project page for electron-store and makes it easier to handle default values, although it does add A LOT of boilerplate code.

A release with my changes is available here: https://github.com/guy0090/loa-details/releases/tag/v0.0.1

karaeren commented 2 years ago

What's the reason for changing the whole settings system, changing hundreds of lines of code for having the exact same effect that is also untested?

guy0090 commented 2 years ago

What's the reason for changing the whole settings system, changing hundreds of lines of code for having the exact same effect that is also untested?

Not sure what you're referring to with untested, it is tested and working in the release I attached to the PR.

I do agree the added lines (mostly the schema, to be fair) may lead to a higher cost to maintain but in the log run this is an easier and more fool-proof way of managing them.

Rewrite may have been the wrong word to describe the change, more than anything it changes it to be inline with the way electron store is meant to be used (schemas over manually merging/saving an entire defaults file every read/write).

A lot of the code in components (setting individual setting options) can be changing into a single function instead of repeating code so I'll add a commit changing that as well. If you'd rather just scrap the entire thing that's obviously up to you to decide.

karaeren commented 2 years ago

It's hard to accept that it's the norm to do it that way when every single settings page needs 200+ lines of changes along with many lines of change on electron source as well... Rest of the pr looks good but I don't really wanna deal with this settings thing in the future when I add a new setting.

Edit: Another note is that this isn't a Typescript project. I've made it in vanilla JS just for simplicity. That's the whole reason this project exists instead of a GUI made in C#/XAML along with the logger.

guy0090 commented 2 years ago

Gotcha, let me send another commit refactoring the way settings are changed in the individual settings components to not require as many lines (that's on me, I just wrote shit code). If it's still not something you're ok with I'll revert it back to the way it was before.

karaeren commented 2 years ago

Thanks for the input but I'd rather have the app settings changes reverted altogether. This kind of big structural change to the whole project shouldn't be squeezed into a PR about another important PR. You need to consider that even though you might have had these tested, I too have to test everything in this PR too before publishing...

guy0090 commented 2 years ago

Got it, that's fine. I'll close this PR and send another one with only uploading included.