NGnius / PowerTools

Moved to
https://git.ngni.us/NG-SD-Plugins/PowerTools
GNU General Public License v3.0
414 stars 29 forks source link

DO NOT MERGE - feat: eslint, prettier, usdpl_front workspace, reducers #51

Closed exdingbat closed 1 year ago

exdingbat commented 1 year ago

i'll update the PR title once i'm confident this is ready for testing

first of all, this plugin is great. definitely a game changer for the deck.

i was going to add come per-cpu-core settings because one of the games i play on the deck through proton works better with logical cores turned off but further cpu affinity tweaking is slightly difficult in the current version of PowerTools...

but i got carried away and ended up refactoring the dev branch instead of adding features. hopefully this refactor is helpful -- if there are parts you disagree with, i'm happy to make changes.

the most notable changes are:

NGnius commented 1 year ago

Wow, thank you for that work! This looks like a pretty big game changer for the PowerTools front-end, so lets call it even on the game-changing :P

I've asked for help reviewing this from members of the Decky discord server, since my TS & React knowledge is pretty basic (hence the need for a refactor, lol). I'm planning on mostly watching from afar and then eventually clicking the nice green "Merge" button once those review(s) have been settled. I trust between you, Decky devs, and me, we can settle those reviews pretty quickly. This is probably the best time to do a big UI change like this, since UI changes are mostly complete for the next update (with the possible exception of a TDP slider, but that's my problem anyway).

I do have a few general questions/housekeeping things to clear up, since this is a pretty major change to the UI:

Have you tested these changes? (Saying no is ok, I just want to know how thoroughly I should test)

Some of the USDPL extensions might be nice to add to USDPL, are you fine with me possibly copying some of that stuff into that project?

cpu affinity tweaking is slightly difficult in the current version of PowerTools

I presume this is in reference to v1.0 which doesn't have the advanced CPU config options, but if you think there's further work to be done there let me know -- I keep getting reminded that Rust is a hard language to learn, so I'm happy to work with you to improve it if you don't want to do it on your own.

Now, since that last paragraph sounds too nice for a FOSS maintainer, I'd like to balance it out by blaming you for something that is competely not your fault: I'm hungry and I've just spent the last while typing this instead of cooking food. Thanks for the late supper and the great code!

exdingbat commented 1 year ago

Wow, thank you for that work! This looks like a pretty big game changer for the PowerTools front-end, so lets call it even on the game-changing :P

thanks!

I've asked for help reviewing this from members of the Decky discord server, since my TS & React knowledge is pretty basic (hence the need for a refactor, lol). I'm planning on mostly watching from afar and then eventually clicking the nice green "Merge" button once those review(s) have been settled. I trust between you, Decky devs, and me, we can settle those reviews pretty quickly. This is probably the best time to do a big UI change like this, since UI changes are mostly complete for the next update (with the possible exception of a TDP slider, but that's my problem anyway).

thank you for taking the time and energy to get more eyes on this PR (i appreciate your time spent reviewing @AAGaming00 ). i realize now that i could have given more context to this PR and asked about whether or not its even an acceptable request! i was in rush and didn't take the time to consider that i was about to dump a bunch of code on a stranger's repo out of the blue.

Have you tested these changes? (Saying no is ok, I just want to know how thoroughly I should test)

i have not tested the changes as they currently exist and i don't think i'll be able to until sunday or monday. i wouldn't want you to waste your time testing it until i'm satisfied these changes don't contain glaring regressions. consider the PR completely untested and possibly broken. i'll comment on the PR with an update once i've done a bit of testing.

Some of the USDPL extensions might be nice to add to USDPL, are you fine with me possibly copying some of that stuff into that project?

💯 copy/move as much as you want!

I presume this is in reference to v1.0 which doesn't have the advanced CPU config options, but if you think there's further work to be done there let me know -- I keep getting reminded that Rust is a hard language to learn, so I'm happy to work with you to improve it if you don't want to do it on your own.

i think i've been using v1.05? whatever version allows controlling SMT and total core count. those controls work great as-is but i wanted to try config with less normal core settings (core 0 off, cores 0 and 1 off, all logical cores and core 0 off). my initial plan was to add toggles for each core to allow independent control of core on/off state. instead, i got carried away refactoring the react frontend.

i would continue with that feature as a followup but it looks redundant with the advanced, per-core controls in the dev branch, which seem to enable that level of discrete control.

Thanks for the late supper

😎

NGnius commented 1 year ago

Hey, what's left to do with this PR?

NGnius commented 1 year ago

Closing as inactive and also because I want to do some UI optimisations during the holidays which will definitely break a lot of this PR.