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

Onboard Manual #379

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Onboard Manual #379

wants to merge 43 commits into from

Conversation

dylwhich
Copy link
Collaborator

No description provided.

main/display/display.c Outdated Show resolved Hide resolved
*/
void drawChar(display_t* disp, paletteColor_t color, int h, const font_ch_t* ch, int16_t xOff, int16_t yOff)
void drawCharItalic(display_t* disp, paletteColor_t color, int h, const font_ch_t* ch, int16_t xOff, int16_t yOff, int8_t slant)
Copy link
Owner

Choose a reason for hiding this comment

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

Also organizationally, I think there's enough text function stuff here to get broken out into it's own file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, display.c has accumulated an awful lot of non-display things. Can move some things to font.c or something.

Copy link
Owner

Choose a reason for hiding this comment

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

I need to impose a "no more than 1k lines" rule or something... MISRA compliance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a mandatory change for this PR to be accepted? I may be able to make spare cycles to do it

main/modes/mode_main_menu.c Outdated Show resolved Hide resolved
main/modes/mode_manual.c Show resolved Hide resolved
main/modes/mode_manual.c Show resolved Hide resolved
{
switch (evt->button)
{
case UP:
Copy link
Owner

Choose a reason for hiding this comment

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

What's the point of chapters if you can only navigate page by page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I was trying to add most recently, but I was doing it in a dumb way and didn't quite have time to finish after I started doing it in a not-dumb way.

main/utils/markdown_parser.c Show resolved Hide resolved
@AEFeinstein
Copy link
Owner

Have you coordinated with thaeli about this?

@Brycey92
Copy link
Collaborator

Have you coordinated with thaeli about this?

I talked to thaeli earlier today, and I got her raw Google form submission csv, so I can make a rough pass at slapping that into markdown formatting tomorrow

@Brycey92
Copy link
Collaborator

Not ready for merge as-is. Newlines are broken, as are horizontal rules. In my testing, the issue persisted before my and Max's changes at 072dc2b, and at db710d8, before merging main. Also tested on onboard-manual-take-2-before-bryce-messed-with-it and issues persisted. Clean builds every time.

Oddly, I went back to each commit on December 6, when dylwhich was showing me those features working, but they were broken on those commits too. It's possible this is an issue on my dev environment, or that dylwhich has uncommitted changes. Can anyone confirm or deny these issues?

Working display for reference
image

@AEFeinstein
Copy link
Owner

I don't have enough time to review and test something this large before magfest, and the actual manual entries aren't written yet, so this is not gonna make the 1.1 cut. I'll leave the PR though. It's definitely something worth fiddling with for next year, though perhaps in a simpler manner.

@Brycey92
Copy link
Collaborator

@dylwhich discovered #386, and I made a PR to fix it in #389. This makes newlines and horizontal rules work when building on Windows.

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

Successfully merging this pull request may close these issues.

4 participants