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

Custom Region Layouts - V2.28.0 breaks functionality #3504

Open
AryasDad opened this issue Jul 20, 2024 · 10 comments
Open

Custom Region Layouts - V2.28.0 breaks functionality #3504

AryasDad opened this issue Jul 20, 2024 · 10 comments

Comments

@AryasDad
Copy link

Hello and thank you for opening an issue.

⚠️ Please make sure that you have read the following lines before submitting your Issue:

I'm not sure if this is a bug

If you're not sure if it's a real bug or if it's just you, please open a topic on the forum: https://forum.magicmirror.builders/category/15/bug-hunt

I'm having troubles installing or configuring MagicMirror

Problems installing or configuring your MagicMirror? Check out: https://forum.magicmirror.builders/category/10/troubleshooting

A common problem is that your config file could be invalid. Please run in your MagicMirror² directory: npm run config:check and see if it reports an error.

I found a bug in the MagicMirror² installer

If you are facing an issue or found a bug while trying to install MagicMirror² via the installer please report it in the respective GitHub repository:
https://github.com/sdetweil/MagicMirror_scripts

I found a bug in the MagicMirror² Docker image

If you are facing an issue or found a bug while running MagicMirror² inside a Docker container please create an issue in the corresponding repository:
https://gitlab.com/khassel/magicmirror

I'm having troubles installing or configuring foreign modules

Please open an issue in the module repository or ask for help in the forum


I found a bug in MagicMirror

Please make sure to only submit reproducible issues. You can safely remove everything above the dividing line.
When submitting a new issue, please supply the following information:

Platform: Intel(R) Client Systems; model: NUC6CAYH, platform: linux; distro: Ubuntu; release: 22.04.4 LTS; arch: x64; kernel: 6.5.0-44-generic, electron: 29.1.6; used node: 20.9.0; installed node: 20.15.1; npm: 10.7.0; pm2: 5.3.1

Node Version: node: 20.15.1

MagicMirror² Version: MagicMirror: v2.27.0 (tried latest 22.28.0 - caused problems)

Description: For approximately the last year if not longer, I have been running my MagicMirror instance successfully with custom layout regions. V2.27.0 appears to be the last release where this worked. V2.28.0 breaks this functionality. I am not sure where I found how to do this, but it was somewhere on the MagicMirror forums. Here is a snippet from my modified index.html:

  <div class="region row3 left">
    <div class="container"></div>
  </div>
  <div class="region row3 right">
    <div class="container"></div>
  </div>

In my config.js, I am able to locate modules in this by using the following:

position: "row3_left"
...
position: "row3_right"

When I updated to V2.28.0, it no longer loads and my error log indicates invalid region position.

Steps to Reproduce: Downgraded to 2.27.0 to get it working again. 2.28.0 breaks it. .

Expected Results: Custom regions in index.html should be recognized by stem like previously..

Actual Results: Multiple errors in log due to "invalid region".

Configuration: pertinent snippet above.

Additional Notes:. I am suspecting the "fix", issue #3445 is what is causing this to no longer work.

My question is, is there a better way to get custom layout functionality that I am unaware of to get my system working with the latest release? The current pre-determined layout is too restrictive and I need more layout options for my system.

@sdetweil
Copy link
Collaborator

@khassel
Copy link
Collaborator

khassel commented Jul 20, 2024

yes, the mentioned PR causes your problems.

AFAIR this was implemented to prevent crashes so we have less support.

@rejas @sdetweil from my side we could add an additional parameter in config.js to allow custom positions so that these (hopefully experienced) users don't have to fix this by changing our code.

@sdetweil
Copy link
Collaborator

@khassel yes prevent crashes but could be an optional property as u suggest. I also suggested code to do discovery, but could be trouble depending, on above

@AryasDad
Copy link
Author

My vote would be to scan the index file for the regions when it does the check. It seems like this ability is already built-in somewhat to the codebase since it was able to use the custom regions before.

However, I always have to remember to replace the index file with my custom after an update, so having the alternative means to add these in via config or other means would also be good. Maybe a way to add a custom section similar to how you can do a custom CSS that gets injected into the index file automagically. That way if an update doesn't wipe out core manual updates.

@sdetweil
Copy link
Collaborator

before we we didn't validate the regions. but if it was wrong, there was a fatal error.. and MM didn't come up
we found a case with no module: and fixed that, and decided to validate the position names too.. this is what caught you..

I did write a sample bit of JS to extract the names from the index.html file.. and ad d them to the list to validate against.
if I can find it again..

@sdetweil
Copy link
Collaborator

const fs = require('fs')
const indexFile =   'index.html'

let lines = fs.readFileSync(indexFile).toString().split('\n')
let regionRegex = /"region ([^"]*)/i

lines.forEach(l =>{
        let results=regionRegex.exec(l)
        if(results && results.length>1){
           console.log("region='"+results[1].replace(' ','_')+"'")
        }
})

@khassel
Copy link
Collaborator

khassel commented Jul 23, 2024

my above PR brings back the situation of v2.27.0, so this would solve this issue but still no solution for the index.html file.

We often had discussions about custom regions, but that was never implemented, e.g. #3231.

If we implement such a solution, it should be robust and not result in increased support.

@sdetweil
Copy link
Collaborator

well, your pr suspends checking. does not check new region names too.

@khassel
Copy link
Collaborator

khassel commented Jul 23, 2024

as I said, brings back the situation of v2.27.0, not more

@sdetweil
Copy link
Collaborator

I thought you would make the property for an array of position names, to add/replace the default ones vs a flag to not check

rejas pushed a commit that referenced this issue Aug 1, 2024
khassel pushed a commit that referenced this issue Aug 27, 2024
…ml (#3518)

read index.html to discover the regions used, make them the list checked
by app.js and check:config test

fixes #3504   supercedes #3506 

no config.js param required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants