Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash possibility if module: <name> is not defined and on mistake position: <position> #3445

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

bugsounet
Copy link
Contributor

Fix #3442

@bugsounet bugsounet marked this pull request as draft May 10, 2024 12:28
@KristjanESPERANTO
Copy link
Contributor

Just a thought: Wouldn't that be a thing that belongs in check_config.js?

@sdetweil
Copy link
Collaborator

yes, also.

@bugsounet
Copy link
Contributor Author

check_config.js check only syntax but it does not check the contents of the file

should we make some other checks on it too ?

@sdetweil
Copy link
Collaborator

sdetweil commented May 13, 2024

well, this one is the first actual content that is important..
altho position should be checked too.. at least valid if specified.. that is another crash..

the user that reporteds the split problem said they ran config:check but it was good..

@bugsounet
Copy link
Contributor Author

bugsounet commented May 13, 2024

So, we have to check module: <name>and position: <valid position> (if defined)

we check it on check_config.js and MM core ?

Personally, I think it's essential to apply the rules to both

@sdetweil
Copy link
Collaborator

I think so.. never crash..

@bugsounet
Copy link
Contributor Author

ok, I will see to code it

@bugsounet
Copy link
Contributor Author

Right, I will try to add needed rules with eslint in check_config.js ;)

I have never done this kind of test.
bugsounet, it's time to learn :)

@bugsounet
Copy link
Contributor Author

bugsounet commented May 18, 2024

module name error:
image

position error:
image

No error:
image

I'm not sure that i can integrate it into eslint

What do you think @sdetweil ?

@bugsounet bugsounet changed the title Fix crash possibility if module: <name> is not defined Fix crash possibility if module: <name> is not defined and on mistake position: <position> May 18, 2024
@bugsounet bugsounet marked this pull request as ready for review May 26, 2024 09:16
@bugsounet
Copy link
Contributor Author

Before approuve some new deps.
Thanks to review this :>

@khassel
Copy link
Collaborator

khassel commented Jun 7, 2024

If I use a module with a valid position and disabled: true , e.g.

		{
			module: "weather",
			header: "default weather module current",
			position: "top_left",
			disabled: true,
			config: {
				weatherProvider: "openmeteo",
				lat: 50.17659,
				lon: 8.62685,
				useCorsProxy: true,
				roundTemp: true,
				showPrecipitationProbability: true,
				showHumidity: "wind",
				showUVIndex: true,
				appendLocationNameToHeader: false,
				windUnits: "kmh",
				showFeelsLike: false,
				bridgeId: "abc",
			},
		},

I get a warning

[2024-06-07 23:21:00.091] [WARN]  No module name found for this configuration: {
  module: 'weather',
  header: 'default weather module current',
  position: 'top_left',
  disabled: true,
  config: {
    weatherProvider: 'openmeteo',
    lat: 50.17659,
    lon: 8.62685,
    useCorsProxy: true,
    roundTemp: true,
    showPrecipitationProbability: true,
    showHumidity: 'wind',
    showUVIndex: true,
    appendLocationNameToHeader: false,
    windUnits: 'kmh',
    showFeelsLike: false,
    bridgeId: 'abc'
  }
}

Is this intended? I think we need no such warning for disabled modules ...

@sdetweil
Copy link
Collaborator

sdetweil commented Jun 8, 2024

i agree, no warning there.

@bugsounet
Copy link
Contributor Author

sure, it's not logical

@bugsounet
Copy link
Contributor Author

fixed

@khassel khassel requested a review from rejas June 8, 2024 18:31
@@ -7,6 +7,7 @@ const Loader = (function () {
const loadedModuleFiles = [];
const loadedFiles = [];
const moduleObjects = [];
const positions = ["top_bar", "top_left", "top_center", "top_right", "upper_third", "middle_center", "lower_third", "bottom_left", "bottom_center", "bottom_right", "bottom_bar", "fullscreen_above", "fullscreen_below"];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code seems double, can we put it in a common class?

@@ -50,7 +51,8 @@ const Loader = (function () {
* @returns {object[]} module data as configured in config
*/
const getAllModules = function () {
return config.modules;
const AllModules = config.modules.filter((module) => (module.module !== undefined) && (positions.indexOf(module.position) > -1 || typeof (module.position) === "undefined"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part of this code seems double, can we put it in a common function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants