openDsh / dash

Join us on Slack! https://join.slack.com/t/opendsh/shared_invite/zt-la398uly-a6eMH5ttEQhbtE6asVKx4Q
GNU General Public License v3.0
238 stars 69 forks source link

plugin support, design refresh, bug fixes, code cleanup, etc #34

Closed rsjudka closed 3 years ago

rsjudka commented 3 years ago

this shouldnt have gotten so big but oh :whale:

dannybarake commented 3 years ago

From my point of view it looks good, ready to merge 👍

rsjudka commented 3 years ago

My only question is - what is this blank rpi.sh? - otherwise looks great.

just made it an executable (chmod +x) weird that it says its empty

icecube45 commented 3 years ago

Ah ok, looks great then.

ZeroErrors commented 3 years ago

I've swapped to testing this branch and seems to be working well. The only issue I've run into is that modified settings won't get saved unless you use the controls bar to exit dash or restart the system.

I guess simply adding a save button on the settings page would work, or maybe saving settings when changing off of the settings page?

icecube45 commented 3 years ago

I thought settings were to be saved on every change, but that might no longer be the case? @ZeroErrors is that just on the settings page, or also on the per tab settings?

ZeroErrors commented 3 years ago

@icecube45 just on the settings page. The settings accessible from the Android Auto page have their own save button but that's only for the openauto.ini from the looks.

Based on what I can find (still learning the codebase) in the code, the old code was saving settings every 10 seconds using a QTimer https://github.com/openDsh/dash/blob/b108083226b1ec5bb58d3e5786b79e3d0127d573/src/app/config.cpp#L48-L50 but that code was removed in this PR https://github.com/openDsh/dash/pull/34/files#diff-5205fd2bb397aca6149095bef3b5ade9214c93f6cba53ee651bdabe8cb9c68abL48-L50

I'm in favor of using something other than a timer for saving anyway since they are very infrequently changed.

icecube45 commented 3 years ago

Yup, you're right. Looks like the only save calls are done on control bar buttons now. Definitely should change that.

rsjudka commented 3 years ago

ah yeah totally forgot about the save button! I was meaning to do the save in the destructor, so basically the config will get saved on closing dash (regardless of how thats initiated)... except now im think the only other way to close dash is by killing it somehow and i dont wanna deal with handling signals lol

so ill definitely need to rethink this, but ill leave that to my next feature (which will basically make the config an accessible "api")

i really dont want a save button tho, trying to keep the config stuff transparent to the user

icecube45 commented 3 years ago

@rsjudka only issue with this then - how do I save my option of not showing the control bar?

rsjudka commented 3 years ago

@icecube45 ah shoot yeah didn't think of that, I'll throw a save button in the settings page for now

Out of curiosity how do you close dash and/or shutdown the pi then?

icecube45 commented 3 years ago

My setup is fully integrated into the car, so when my car's ignition state goes low, it triggers a graceful shutdown of the pi (after a 4 minute buffer period to account for quick stops).

So, a graceful killing of dash.