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(color): Fix handling of blank lines in YAML files. #3762

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

philmoz
Copy link
Collaborator

@philmoz philmoz commented Jul 5, 2023

Fixes #3760

Better handling of blank lines in YAML files in the case where the blank line was at the start of the parse buffer after a newline ended the previous buffer.

@raphaelcoeffic
Copy link
Member

We should really add a lot of unit tests to this code.... I'm afraid we'll run into other issues otherwise.

@philmoz
Copy link
Collaborator Author

philmoz commented Jul 5, 2023

Model and radio files have not had blank lines that I am aware of.
It is only the theme files that tended to have these.

@pfeerick
Copy link
Member

pfeerick commented Jul 5, 2023

Part of the problem was probably the assumption re: line endings... i.e. the theme file from #3760 actually worked fine for me fine in the simulator without removing the line... but that was because I copied the contents into a new file as I was going to remove that blank line, so the line endings were probably changed from UNIX/MAC to Windows style. I can repeatably break and unbreak it now by simply changing the line endings and resaving the file, with the blank line still there.

Either way, now I'm going to check all the themes in the themes directory to make sure there are none with a blank line in them 😆

pfeerick added a commit to EdgeTX/themes that referenced this pull request Jul 5, 2023
@philmoz philmoz changed the title fix(color) - Fix handling of blank lines in YAML files. fix(color): Fix handling of blank lines in YAML files. Jul 6, 2023
@pfeerick pfeerick self-requested a review July 30, 2023 05:43
@raphaelcoeffic raphaelcoeffic self-assigned this Aug 12, 2023
@raphaelcoeffic
Copy link
Member

@pfeerick I assigned this PR to me to force me to add some unit testing of these glare conditions.

@pfeerick pfeerick added bug/regression ↩️ A new version of EdgeTX broke something needs: unit tests Unit tests are needed to for continuous validation labels Aug 28, 2023
@raphaelcoeffic
Copy link
Member

@philmoz I finally managed to make some basic unit tests that work with your fix but fail with main. That's all the proof I needed. It does not seem to break anything else either.

@raphaelcoeffic raphaelcoeffic removed the needs: unit tests Unit tests are needed to for continuous validation label Sep 2, 2023
@raphaelcoeffic
Copy link
Member

@pfeerick it's planned to cherry-pick into 2.9, right?

@pfeerick
Copy link
Member

pfeerick commented Sep 4, 2023

Yes, that was the intention, as it was needed to prevent old themes from turning the UI into an un-navigable black hole 😆

@pfeerick pfeerick modified the milestones: 2.9, 2.9.x Sep 4, 2023
@pfeerick pfeerick merged commit 65ee80f into EdgeTX:main Sep 7, 2023
pfeerick added a commit that referenced this pull request Sep 20, 2023
* Fix handling of blank lines in YAML files.

* Added some basic EOL / blank line unit tests

---------

Co-authored-by: raphaelcoeffic <raphael.coeffic@frafos.com>
@claudevandaele
Copy link

I can inform you that old themes still cause a black screen when installing 2.9.1. I've had this happen with an older theme so I had to use a backup to get out of the black screen. Greats Claude

@philmoz
Copy link
Collaborator Author

philmoz commented Oct 17, 2023

If you have a theme that does this please upload a copy of it here.

@claudevandaele
Copy link

background_480x272
logo
screenshot1
screenshot2
screenshot3

@philmoz
Copy link
Collaborator Author

philmoz commented Oct 18, 2023

You can download an updated version of that theme from https://github.com/EdgeTX/themes

@claudevandaele
Copy link

Thanks !

@philmoz philmoz deleted the fix-yaml-blank-line-parse branch December 11, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/regression ↩️ A new version of EdgeTX broke something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.9 Themes
4 participants