WIP: Super-rough structure-only per-screen refactor #219

2025-01-06T23:59:27Z

sjoerdverweij commented

2025-01-06 23:59:27 +00:00

(Migrated from github.com)

No description provided.

learn-more (Migrated from github.com) reviewed

2025-01-08 08:27:09 +00:00

firmware/src/core/Feed.h

					
				@ -0,0 +1,21 @@

				#ifndef FEED_H

I would suggest not to use a dynamic_cast.
A static_cast + a check on the correct type (id) would be a lot faster.

A dynamic_cast is a very slow function, that requires a certain amount of debug info to be present in the binary.

I would suggest not to use a `dynamic_cast`. A `static_cast` + a check on the correct type (id) would be a lot faster. A `dynamic_cast` is a very slow function, that _requires_ a certain amount of debug info to be present in the binary.

Is the name required?
I'd say that 'id' would be enough.

and a nit: this is indented with 3 spaces.

Is the name required? I'd say that 'id' would be enough. and a nit: this is indented with 3 spaces.

No need to make it virtual, just have a const FeedId Id in the class, and create a constructor that you need to pass the FeedId into:

struct Feed {
    const FeedId Id;
    explicit Feed(FeedId id)
        : Id(id)
    {
    }
};

No need to make it virtual, just have a `const FeedId Id` in the class, and create a constructor that you need to pass the `FeedId` into: ```cpp struct Feed { const FeedId Id; explicit Feed(FeedId id) : Id(id) { } }; ```

public is not needed in a struct as first item, everything in a struct is public by default

`public` is not needed in a struct as first item, everything in a struct is public by default

firmware/src/core/Feeds.h

					
				@ -0,0 +1,28 @@

				#ifndef FEEDS_H

std::map or std::unordered_map would be better here,
since it does not force you to pre-size it.

`std::map` or `std::unordered_map` would be better here, since it does not force you to pre-size it.

You need to use forward slashes, linux chokes on the backslash, windows does not care so both work there.

firmware/src/core/Feeds.h

					
				@ -0,0 +17,4 @@

				        _Feeds.insert(std::make_pair(feedId, newFeed));

				    }

				    return *_Feeds[feedId];

				  }

You should return something here, or remove the return type

firmware/src/core/Feeds.h

					
				@ -0,0 +20,4 @@

				  }

				  Feed& getFeed(const FeedId feedId) {

				    assert(_Feeds[feedId]);

This suggests that all feeds are always created, I would advice against that.
Anything that is not needed, does not have to be loaded.

This suggests that all feeds are always created, I would advice against that. Anything that is not needed, does not have to be loaded.

firmware/src/core/Orb.h

					
				@ -0,0 +13,4 @@

				    OrbWeatherIcon = 3,

				    OrbTemperatureHighLow = 4,

				    OrbWeather3DayForecast = 5

				};

Something like this allows to mix and match the orbs, which is a long-standing feature request:

enum class OrbId
{
    Invalid,

    Weather_DateTimeWeekday,
    Weather_CityPrediction,
    Weather_Icon,
    Weather_TemperatureHighLow,
    Weather_3DayForecast,

    Stocks_1,
    Stocks_2,
    Stocks_3,
    Stocks_4,
    Stocks_5,

    Clock_Hour_h,
    Clock_Hour_l,
    Clock_SecondsColon,
    Clock_Minute_h,
    Clock_Minute_l,
};

Something like this allows to mix and match the orbs, which is a long-standing feature request: ```cpp enum class OrbId { Invalid, Weather_DateTimeWeekday, Weather_CityPrediction, Weather_Icon, Weather_TemperatureHighLow, Weather_3DayForecast, Stocks_1, Stocks_2, Stocks_3, Stocks_4, Stocks_5, Clock_Hour_h, Clock_Hour_l, Clock_SecondsColon, Clock_Minute_h, Clock_Minute_l, }; ```

dreed47 commented

2025-01-08 19:26:43 +00:00

(Migrated from github.com)

I like the overall design direction. I have a couple logical model recommendations.

A Gadget represents 5 Orbs but as I see it it needs to support both pre-defined Orb sets like clock and also mix/match user defined Orb sets. I'd recommend abstracting out a ContentSet class. Then maybe rename Gadget to ContentManager. So the ContentManager manages a collection of ContentSets and allow switching between them. So the ContentManager manages both predefined and user-defined ContentSets, handle button interations to cycle through the ContentSets and provides methods for creating, updating, and saving user-defined ContentSets.
Feeds - The ContentManager (Gadget) should manage the lifecycle of the Feeds, ensure that Feeds are shared across all Orbs to reduce redundant data fetching, and dynamically load Feeds when needed by one or more Orbs. Also Feeds should make use of TaskManager/TaskFactory when getting data from external APIs
Button Handling - ContentManager (Gadget) should specifically manage cycling of ContentSets via left/right button clicks.
Renderer - The ContentManager (gadget) should coordinate with the Renderer to draw the active ContentSet on the 5 displays. Each Orb in the active ContentSet should use the Renderer to draw its content on the appropriate display.
Need support for persistence for User-Defined ContentSets? to non-volatile storage? then would need methods for loading saved ContentSets on startup

I like the overall design direction. I have a couple logical model recommendations. 1. A Gadget represents 5 Orbs but as I see it it needs to support both pre-defined Orb sets like clock and also mix/match user defined Orb sets. I'd recommend abstracting out a ContentSet class. Then maybe rename Gadget to ContentManager. So the ContentManager manages a collection of ContentSets and allow switching between them. So the ContentManager manages both predefined and user-defined ContentSets, handle button interations to cycle through the ContentSets and provides methods for creating, updating, and saving user-defined ContentSets. 2. Feeds - The ContentManager (Gadget) should manage the lifecycle of the Feeds, ensure that Feeds are shared across all Orbs to reduce redundant data fetching, and dynamically load Feeds when needed by one or more Orbs. Also Feeds should make use of TaskManager/TaskFactory when getting data from external APIs 3. Button Handling - ContentManager (Gadget) should specifically manage cycling of ContentSets via left/right button clicks. 4. Renderer - The ContentManager (gadget) should coordinate with the Renderer to draw the active ContentSet on the 5 displays. Each Orb in the active ContentSet should use the Renderer to draw its content on the appropriate display. 5. Need support for persistence for User-Defined ContentSets? to non-volatile storage? then would need methods for loading saved ContentSets on startup

sjoerdverweij commented

2025-01-09 17:32:40 +00:00

(Migrated from github.com)

I like the overall design direction. I have a couple logical model recommendations.

A Gadget represents 5 Orbs but as I see it it needs to support both pre-defined Orb sets like clock and also mix/match user defined Orb sets. I'd recommend abstracting out a ContentSet class. Then maybe rename Gadget to ContentManager. So the ContentManager manages a collection of ContentSets and allow switching between them. So the ContentManager manages both predefined and user-defined ContentSets, handle button interations to cycle through the ContentSets and provides methods for creating, updating, and saving user-defined ContentSets.

Feeds - The ContentManager (Gadget) should manage the lifecycle of the Feeds, ensure that Feeds are shared across all Orbs to reduce redundant data fetching, and dynamically load Feeds when needed by one or more Orbs. Also Feeds should make use of TaskManager/TaskFactory when getting data from external APIs

Button Handling - ContentManager (Gadget) should specifically manage cycling of ContentSets via left/right button clicks.

Renderer - The ContentManager (gadget) should coordinate with the Renderer to draw the active ContentSet on the 5 displays. Each Orb in the active ContentSet should use the Renderer to draw its content on the appropriate display.

Need support for persistence for User-Defined ContentSets? to non-volatile storage? then would need methods for loading saved ContentSets on startup

Gadget only needs to support configurable mix/match orb sets, because there really no longer is such a thing as a predefined widget; those are just default mix/match configurations. You'd start out with a configuration for a Gadget that has 4 instances of a ClockDigit orb, and one ClockSeconds orb. If we want to prevent users from changing the default settings, we can make them read-only -- but they will still work the exact same way.
Lifecycle and deduplication of Feeds is in the Feeds class. It should be a singleton, which it currently is not properly set up and referenced as.
Button handling etc.; I think Gadgets is what you want ContentManager to be.
Agreed, it will, the draw code isn't fleshed out yet.
I'm open to the best way to save settings. My current thinking is Serialize/Deserialize methods on the top of the settings hierarchy (Settings) which (de)serializes the global settings, then calls all its GadgetSettings, which then calls all its OrbSettings. (GadgetSettings holds which orbs a gadget shows, OrbSettings has the specific settings for that instance of the orb).

> I like the overall design direction. I have a couple logical model recommendations. > > 1. A Gadget represents 5 Orbs but as I see it it needs to support both pre-defined Orb sets like clock and also mix/match user defined Orb sets. I'd recommend abstracting out a ContentSet class. Then maybe rename Gadget to ContentManager. So the ContentManager manages a collection of ContentSets and allow switching between them. So the ContentManager manages both predefined and user-defined ContentSets, handle button interations to cycle through the ContentSets and provides methods for creating, updating, and saving user-defined ContentSets. > 2. Feeds - The ContentManager (Gadget) should manage the lifecycle of the Feeds, ensure that Feeds are shared across all Orbs to reduce redundant data fetching, and dynamically load Feeds when needed by one or more Orbs. Also Feeds should make use of TaskManager/TaskFactory when getting data from external APIs > 3. Button Handling - ContentManager (Gadget) should specifically manage cycling of ContentSets via left/right button clicks. > 4. Renderer - The ContentManager (gadget) should coordinate with the Renderer to draw the active ContentSet on the 5 displays. Each Orb in the active ContentSet should use the Renderer to draw its content on the appropriate display. > 5. Need support for persistence for User-Defined ContentSets? to non-volatile storage? then would need methods for loading saved ContentSets on startup Gadget only needs to support configurable mix/match orb sets, because there really no longer is such a thing as a predefined widget; those are just default mix/match configurations. You'd start out with a configuration for a Gadget that has 4 instances of a ClockDigit orb, and one ClockSeconds orb. If we want to prevent users from changing the default settings, we can make them read-only -- but they will still work the exact same way. Lifecycle and deduplication of Feeds is in the Feeds class. It should be a singleton, which it currently is not properly set up and referenced as. Button handling etc.; I think Gadgets is what you want ContentManager to be. Agreed, it will, the draw code isn't fleshed out yet. I'm open to the best way to save settings. My current thinking is Serialize/Deserialize methods on the top of the settings hierarchy (Settings) which (de)serializes the global settings, then calls all its GadgetSettings, which then calls all its OrbSettings. (GadgetSettings holds which orbs a gadget shows, OrbSettings has the specific settings for that instance of the orb).

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 17:52:02 +00:00

firmware/src/core/Feeds.h

					
				@ -0,0 +1,28 @@

				#ifndef FEEDS_H

When I wrote that bit I didn't know std:: was in play! Huzzah!

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 18:26:42 +00:00

firmware/src/core/Feeds.h

					
				@ -0,0 +20,4 @@

				  }

				  Feed& getFeed(const FeedId feedId) {

				    assert(_Feeds[feedId]);

No, it asserts only if someone asks for a feed from the collection that was not added. For example, someone writing a custom Orb that asks for a feed that it didn't register for. Should be a compile-time oopsie only, hence the assert.

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 18:27:45 +00:00

firmware/src/core/Feed.h

					
				@ -0,0 +1,21 @@

				#ifndef FEED_H

I forget that all the time, so I like to be explicit. If we decide it's an enforced style thing, no problem, I'll take it out.

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 18:28:24 +00:00

firmware/src/core/Feed.h

					
				@ -0,0 +1,21 @@

				#ifndef FEED_H

We'll need some kind of name to show in the config UI. Whether this is the best way to do it is debatable, I am open to suggestions.

(Alternately, we could have a big switch somewhere that names Ids, but that makes it easy for Orb authors to forget...)

We'll need some kind of name to show in the config UI. Whether this is the best way to do it is debatable, I am open to suggestions. (Alternately, we could have a big switch somewhere that names Ids, but that makes it easy for Orb authors to forget...)

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 18:31:10 +00:00

firmware/src/core/Orb.h

					
				@ -0,0 +13,4 @@

				    OrbWeatherIcon = 3,

				    OrbTemperatureHighLow = 4,

				    OrbWeather3DayForecast = 5

				};

Yes, that's the point of it, I just started with the weather widget -- are you saying you don't like the naming of the enum members?

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 18:40:15 +00:00

firmware/src/core/Feed.h

					
				@ -0,0 +1,21 @@

				#ifndef FEED_H

Good point, done.

sjoerdverweij (Migrated from github.com) reviewed

2025-01-09 18:40:48 +00:00

firmware/src/core/Feed.h

					
				@ -0,0 +1,21 @@

				#ifndef FEED_H

Good point, done. Not even going to check on type, this would be very much caught during dev.

learn-more (Migrated from github.com) reviewed

2025-01-09 23:34:43 +00:00

firmware/src/core/Orb.h

					
				@ -0,0 +13,4 @@

				    OrbWeatherIcon = 3,

				    OrbTemperatureHighLow = 4,

				    OrbWeather3DayForecast = 5

				};

I guess so.

In cases like this, having a consistent style in naming pays off imo.

I guess so. In cases like this, having a consistent style in naming pays off imo.

lyricnz commented

2025-01-10 05:07:53 +00:00

(Migrated from github.com)

Sorry I broke this PR merging #218

lyricnz (Migrated from github.com) reviewed

2025-01-10 05:24:47 +00:00

firmware/src/core/Orb.h

					
				@ -0,0 +13,4 @@

				    OrbWeatherIcon = 3,

				    OrbTemperatureHighLow = 4,

				    OrbWeather3DayForecast = 5

				};

Why would we need an enum? Why not a list of list-of instances? (with constuctor/factory parameters as required). Totally value pseudo-code

[
  [
    new Clock(mode=HourH),
    new Clock(mode=HourL),
    new Clock(mode=SecondsColon),
    new Clock(mode=SecondsH),
    new Clock(mode=SecondsL)
  ],
  [
    new Stock(symbol=whatever, currency=whatever, apikey=whateever),
    ...
  ],
  [
    new WeatherTemp(..., location=whatever),
    new WeatherRain(..., location=whatever),
    new WeatherThing(..., location=whatever),
    ...
  ]
]

Why would we need an enum? Why not a list of list-of instances? (with constuctor/factory parameters as required). Totally value pseudo-code ```cpp [ [ new Clock(mode=HourH), new Clock(mode=HourL), new Clock(mode=SecondsColon), new Clock(mode=SecondsH), new Clock(mode=SecondsL) ], [ new Stock(symbol=whatever, currency=whatever, apikey=whateever), ... ], [ new WeatherTemp(..., location=whatever), new WeatherRain(..., location=whatever), new WeatherThing(..., location=whatever), ... ] ] ```

I guess we need something in the UI, ya

learn-more (Migrated from github.com) reviewed

2025-01-13 14:52:24 +00:00

firmware/src/core/Orb.h

					
				@ -0,0 +13,4 @@

				    OrbWeatherIcon = 3,

				    OrbTemperatureHighLow = 4,

				    OrbWeather3DayForecast = 5

				};

In the UI, and in the config files / web editor / whatever is used to configure this thing.
You can't really transmit class instances over the web ;)

In the UI, and in the config files / web editor / whatever is used to configure this thing. You can't really transmit class instances over the web ;)

This pull request has changes conflicting with the target branch.

firmware/src/widgets/weatherwidget/WeatherWidget.h

View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Merge commit title

Merge pull request 'WIP: Super-rough structure-only per-screen refactor' (#219) from sjoerdverweij/dev into dev

Merge commit body

Reviewed-on: https://truesilver.dev/greg/info-orbs/pulls/219

Merge pull request 'WIP: Super-rough structure-only per-screen refactor' (#219) from sjoerdverweij/dev into dev

Reviewed-on: https://truesilver.dev/greg/info-orbs/pulls/219

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin sjoerdverweij/dev:sjoerdverweij/dev

git switch sjoerdverweij/dev

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch dev

git merge --no-ff sjoerdverweij/dev

git switch sjoerdverweij/dev

git rebase dev

git switch dev

git merge --ff-only sjoerdverweij/dev

git switch sjoerdverweij/dev

git rebase dev

git switch dev

git merge --no-ff sjoerdverweij/dev

git switch dev

git merge --squash sjoerdverweij/dev