Settings and Actions Toolbar

[DXCanas]

WIP, first code contribution! Trying out an idea I had based on need expressed here:

Functionally: I hate wasting a tile on command buttons I use once per training (at a maximum).

#168 (reply in thread)

Also corrected a quirk in heigh calculation that led to the tiniest bit of scrolling.

Testing TBD, will try it on next workout.

[Abasz]

This is brilliant, and its hilarious why I have not thought of this when I worked on the tiles years ago :D We could even hide it dynamically like windows task bar (though this will require some gesture support for touchscreens or we just show it on any click anywhere that also works) on smaller screens.

[JaapvanEkris]

Great work! What a space saver! I'll test this evening and accept the request when no issues are found

[Abasz]

Can it be that something is wrong with the slot tracker:

I can see that there is no option for action yet its here. I am suspecting we will need to clear (preferably update) local storage where these settings are saved. So we should run a simple migration I suppose where we swap out the actions to something else.

[DXCanas]

I think new defaults are needed. And the local storage carries over. I could have sworn I removed it from the list, and I still haven’t removed the component from the code base 😅

Tested it myself during a workout today and I did notice some quirks. But overall seemed mostly functional! 12x on the tablet was glorious.

[Abasz]

You removed it from the defaults but since localStorage has it it will happily load it back:

https://github.com/DXCanas/openrowingmonitor/blob/6bde3fa16425c2c3e07dab39a7069685a214cc79/app/client/components/SettingsDialog.js#L160C4-L160C62

Here basically we just get whatever is in the store.

https://github.com/DXCanas/openrowingmonitor/blob/6bde3fa16425c2c3e07dab39a7069685a214cc79/app/client/index.js#L29

here we load it from the store.

    const config = this._appState.config.guiConfigs
    Object.keys(config).forEach(key => {
      config[key] = JSON.parse(localStorage.getItem(key)) ?? config[key]
    })

We need a settings migration here, namely that if config has actions we need to override it and write this new config back to local storage.

[DXCanas]

Heads up that I am also unhappy with the state of the CSS here; it looks ok but it's not very understandable atm. So I will probably be tinkering and testing that along with the state reset ^

[JaapvanEkris]

Before I forget: One thing to look at is the update behaviour of the force curve. We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve), but it keeps drawing CPU cycles when displayed. Perhaps it is redrawing too often?

[DXCanas]

Alright y'all — sorry, I got a little carried away with the CSS. If the changes are out of scope, will happily pull into another PR.

Tried to make the CSS more self explanatory, and addressed an iOS bug I ran into while working on something else.

It's stricter/more defined now, and no longer has what I believe was unintentional overflow/scrolling.

So. Here's the before (Saw this while testing #171 and prior):

And here it is now (only tested locally):

[DXCanas]

Before I forget: One thing to look at is the update behaviour of the force curve. We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve), but it keeps drawing CPU cycles when displayed. Perhaps it is redrawing too often?

Relying heavily on the AI for this, but the solution makes sense to me!

[JaapvanEkris]

We need a settings migration here, namely that if config has actions we need to override it and write this new config back to local storage.

Or we need to clear the browser cache on update.

There are some PR's regarding the cache and the browser anyway (if the user isn't named 'pi' there are install issues, and we might switch browser to something lighter as on the Zero2W there are memory issues), so clearing it will kill any CSS and caching issues?

[Abasz]

Or we need to clear the browser cache on update.

I think that is a bit heavy. I like the current migration. I think it is sufficient.

We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve),

So this is one of the things that proper data layer would solve. When I was experimenting with rxjs it has distintcUntilChanged which is a breez (so I was checking data on each tile for changes and only update/render).

Actually you should not worry about this for now, as I will review the updates to the lit framework and rework this data layer which will improve change detection (and trigger less rendering). I did more reading and it is going to be gradual but still robust. It will remove this passing down global state and make it more focused, that will reduce re-rendering.

The issue currently is that we use the global state which is fully replaced on every new incoming data packet (250ms as per default gui update interval I think), hence triggering all components update. I think moving to update() will not help but you can check that with a logging statement (i.e. how many times the update() method runs).

[JaapvanEkris]

A practical remark/question: currently we have lit pinned to 2.8, as upgrading to 3.x will break the FE. If you want a higher version, just drop a line. If we can uograde to 3.x pkease also tell as it might unlock a bunch of dependent package updates.

[JaapvanEkris]

Or we need to clear the browser cache on update.

I think that is a bit heavy. I like the current migration. I think it is sufficient.

You realize we might change browser or cach store completely due to the outstanding issues (thus clearing any cache anyway)?

[Abasz]

Or we need to clear the browser cache on update.

I think that is a bit heavy. I like the current migration. I think it is sufficient.

You realize we might change browser or cach store completely due to the outstanding issues (thus clearing any cache anyway)?

Yes that is true and essentially it will result in the same effect for those that use a screen with the Pi. But we need to keep it mind that

1. this migration code will remain there for later and potentially be extended with other migration
2. what if someone would already use the GUI on a phone and access it via wifi (i.e. not locally), or from another devices. For them full clear is total overkill.

[JaapvanEkris]

2. what if someone would already use the GUI on a phone and access it via wifi (i.e. not locally), or from another devices. For them full clear is total overkill.

Forgot about that one. Thanks!

[DXCanas]

Alright, given how much back and forth is going on after trying to address this:

One thing to look at is the update behaviour of the force curve. We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve), but it keeps drawing CPU cycles when displayed. Perhaps it is redrawing too often?
#170 (comment)

I thing we should just tighten the scope here and pull out all of that code for another PR. Seems like there are lots of things that need to be validated or confirmed, including what constitutes "fighting the framework" and benchmarking performance.