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

Update Screen.cpp - Sleeping screen with owner.short_name #4109

Closed
wants to merge 10 commits into from

Conversation

Dorn8010
Copy link
Contributor

Added owner.short_name to "Sleeping" screen message. Didn't check variable availablility - not C++ experienced. :/
Pls recheck.

Thank you for sending in a pull request, here's some tips to get started!

(Please delete all these tips and replace with your text)

  • Before starting on some new big chunk of code, it it is optional but highly recommended to open an issue first
    to say "hey, I think this idea X should be implemented and I'm starting work on it. My general plan is Y, any feedback
    is appreciated." This will allow other devs to potentially save you time by not accidentially duplicating work etc...
  • Please do not check in files that don't have real changes
  • Please do not reformat lines that you didn't have to change the code on
  • We recommend using the Visual Studio Code editor along with the 'Trunk Check' extension (In beta for windows, WSL2 for the linux version),
    because it automatically follows our indentation rules and its auto reformatting will not cause spurious changes to lines.
  • If your PR fixes a bug, mention "fixes #bugnum" somewhere in your pull request description.
  • If your other co-developers have comments on your PR please tweak as needed.
  • Please also enable "Allow edits by maintainers".

@CLAassistant
Copy link

CLAassistant commented Jun 14, 2024

CLA assistant check
All committers have signed the CLA.

@todd-herbert todd-herbert self-requested a review June 15, 2024 01:05
Copy link
Contributor

@todd-herbert todd-herbert left a comment

Choose a reason for hiding this comment

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

Looks good on my devices 👍

// Create a new string that is the concatenation of idText and " sleeping."
size_t idTextLen = strlen(idText);
char displayStr[idTextLen + 12]; // Allocate enough space for the new string
strcpy(displayStr, idText); // Copy idText to the new string
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps more consolidated to write in this way with a single expression?
sprintf(displayStr, "%s sleeping.", owner.short_name);

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, no need to declare idText this way. You can just reference owner.short_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely guys. I'm not a C++ programmer and just tried that the code works. I also don't like the strlen check on a maybe unassigned var. So if you guys know it better - pls change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about something like
if (!idText || strlen(idText) == 0) {
for the check of idText ?

Copy link
Contributor

@todd-herbert todd-herbert Jun 15, 2024

Choose a reason for hiding this comment

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

Actually, is it possible to get into a situation where short name is has zero length? It might not be an issue? Now I think about it though, I'm not sure what would happen with emoji short names. I should check if this is a problem with the existing E-Ink screensaver code too.

Update: okay yeah, emoji short names cause weird stuff here, but the same issue shows up with existing code too. I'll see if there's any convenient way to handle it.

@todd-herbert
Copy link
Contributor

Just an update: the new haveGlyphs function will help you catch any unprintable characters.
Something like:

if (haveGlyphs(owner.short_name)) {
    // use the short name
}
else {
    // don't use the short name
}

A bit of house keeping: can we also move the haveGlyphs definition inside #ifdef USE_EINK eventually? Just to get rid of compiler warnings that it's not used.

Added owner.short_name to "Sleeping" screen message. Didn't check variable.
Another try :)
Really last try :=
@Dorn8010 Dorn8010 changed the title Update Screen.cpp Update Screen.cpp - Sleeping screen with owner.short_name Jun 16, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idText used for common style with the following drawScreensaverOverlay.
snprintf for mem safety and 30 chars gives enough space for later changes of text

@Dorn8010
Copy link
Contributor Author

Just an update: the new haveGlyphs function will help you catch any unprintable characters. Something like:

if (haveGlyphs(owner.short_name)) {
    // use the short name
}
else {
    // don't use the short name
}

A bit of house keeping: can we also move the haveGlyphs definition inside #ifdef USE_EINK eventually? Just to get rid of compiler warnings that it's not used.

Waited with this. If OK for you guys, I would add the short_name and HEX ID at the boot screen e.g. EU_868:ad24:!A1B2C3 - so every device can immediately be identified at startup and the unique ID is visible - and then we need this haveGlyphs also when using the other devices.
For logical naming - I would like to change the name of the functinon into haveNoGlyphs - since a true answer would mean exactly this.

Copy link
Contributor

@todd-herbert todd-herbert left a comment

Choose a reason for hiding this comment

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

If OK for you guys, I would add the short_name and HEX ID at the boot screen e.g. EU_868:ad24:!A1B2C3

No strong opinion on this one!

and then we need this haveGlyphs also when using the other devices.

It's possible, although I'm not sure about the idea. We're hiding the missing characters here for cosmetic purposes, but in some other situations, it might be important that we continue to identify them with ¿.

src/graphics/Screen.cpp Show resolved Hide resolved
src/graphics/Screen.cpp Show resolved Hide resolved
@Dorn8010 Dorn8010 closed this Jun 18, 2024
@Dorn8010 Dorn8010 deleted the patch-1 branch June 18, 2024 09:24
@todd-herbert
Copy link
Contributor

I still think this is a great idea. Keen to see it included!

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.

[Feature Request]: Write owner.short_name " sleeping..." into device sleep screen (useful for e-paper)
4 participants