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

Implement optional looping for animated boot logo [#1839] #1841

Merged
merged 17 commits into from
Nov 23, 2023

Conversation

ia
Copy link
Collaborator

@ia ia commented Nov 20, 2023

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?
    Implementation of [optional] looping for animated boot logo. Related feature request is here.

  • What is the current behavior?
    If the boot logo is animated & the option is set to infinite, then animation showed only once.

  • What is the new behavior (if this is a feature change)?
    If the boot logo is animated & the setting is to infinite, then animation is being shown on repeat until a button press. To set "old" behavior, use "1" option value (as in one time) for the animation setting in the menu. No changes for static logo: both "1" and "infinity" options do the same for a static logo as before.

  • Other information:
    All builds & autotests has been successfully (local runs). Tested only on TS80P real hardware. Seems working. In addition to the main request, tried to improve readability of related sections. As always, can't wait any feedback.

@discip
Copy link
Collaborator

discip commented Nov 20, 2023

@ia
My only complaint is with the way this is implemented. The term “duration” no longer applies, as the difference between option “1” and “∞” only concerns the way the animation is done.

@ia
Copy link
Collaborator Author

ia commented Nov 20, 2023

The term “duration” no longer applies, as the difference between option “1” and “∞” only concerns the way the animation is done.

I was thinking about that too. Well, yes and no. "Duration" just becomes not only as how much in seconds as in "1s...4s" but as how much in times, i.e. show only one time or show infinite amount of times (kinda duration). I understand it may not sound smooth but reworking settings, menu items, text & translations just doesn't cost the effort of this little GUI feature (which a user will see only once on start-up) in my opinion. 👉 👈

That's why I tried to make this little feature as less invasive as possible but, yea, even with such little things the more you work on them the more nuances are coming up on the surface.

Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

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

Edit: removed old comment - mis-read the code.

We still need to support timeouts + loop options here.
I would suggest keeping the timeout in seconds but having two infinite settings "infinite-hold" and "infinite-loop"
Can probably use a custom logo for these to distinguish these on the UI similar to the repeat-all and repeat-one icons people use for music playback?

@ia
Copy link
Collaborator Author

ia commented Nov 21, 2023

We still need to support timeouts + loop options here.

Timeouts are not going anywhere, it's still supported. I just did double-checked it now.

I would suggest keeping the timeout in seconds but having two infinite settings "infinite-hold" and "infinite-loop"

But if I understand you correctly, that's how exactly it works now. Okay, just in case, so all of us could be on the same page (me included so I wouldn't confuse myself) here is the brief table for this option as it's implemented in this patch already:

logo\option off 1s...4s 1 inf
static skip show for N s, then skip show until button press show until button press
animated skip show for N s, then skip (if animation is shorter then N, then halt with the last frame until full timeout) show animation once, then halt with the last frame until button press show animation on repeat until button press

I think it's reasonable set of options/behavior and it shouldn't break any previous settings existed for this option (except that now infinity means "infinite animation for animated logo"). At the same time I hope there won't be demand for the case like "I want to see animation in loop but only for 4 seconds" because I suppose it doesn't make much of a sense since duration of animated logos (at least most of those which are pre-rendered in the meta repo) which I took for tests is just about 2-3 seconds.

Can probably use a custom logo for these to distinguish these on the UI

However, this is what I really would like to avoid somehow not to:

  • overblown firmware with just one icon taking a lot of space;
  • giving too much time & effort for a such little feature.

That's why for Proof-of-Concept patch I just picked "1" as in one-time. It could be a good compromise, if someone would give me a hint what exactly most suitable chars/numbers/logos which are already in the firmware can be combined & reused to be drawn as a setting for one-time animation. One of the most obvious things to try first is something like "1∞" if "1" is not good enough. :D

similar to the repeat-all and repeat-one icons people use for music playback?

Funny that you mentioned that since it flashed in my head while I was testing this patch on the device changing the related setting. That's why I picked "1". And just like in a music player, whether you repeat a playlist with one track or one track from playlist with one track, the result is the same just like if we have one frame of static image so 1/inf may be confusing a bit at first but I hope there is some sense in it - whether we roll one frame of static image one time or indefinitely, the result is the same. :)

@discip
Copy link
Collaborator

discip commented Nov 21, 2023

Can probably use a custom logo for these to distinguish these on the UI similar to the repeat-all and repeat-one icons people use for music playback?

I had something similar in mind, 😀 but was not sure because of the fact I mentioned above.

Maybe I'll post a sketch later, when I am back home.

@ia
Copy link
Collaborator Author

ia commented Nov 21, 2023

Experimented a bit with the infinity icon. Plus decided to check if will it be look like repeat circle but without an arrow if I just slice its pixels in half. Decide yourself. I think this is the best suggestions which I can come up with so far.

Sorry for the horrible photo quality though but I hope you got the idea.

Should I commit any variant in case if someone is interested in testing particular one?


1∞
1∞


1○
1○



○


@discip
Copy link
Collaborator

discip commented Nov 21, 2023

@ia
As you might have seen, I created a PR to update the current infinity icon. ia#1
And it even fits the TS80P.

I hope you like them:
infinity-1
infinity-loop

@ia
Copy link
Collaborator Author

ia commented Nov 21, 2023

@ia As you might have seen, I created a PR to update the current infinity icon. ia#1 And it even fits the TS80P.

First of all, thanks for the help with this. Second, sorry for a delayed response because for some reason I didn't get any notification on this pull-request there - just by accident noticed pull-request when manually went to web page with my fork on github. Was going to respond you there but doing it here as main place for discussion & feedback.

I hope you like them

Third, I really like 1+inf logo... but don't you think that the other one with repeating circle with arrow has redundant information? Infinity by its logo means that it's infinite but with the repeat sub-logo it looks like we're repeating infinity? I mean, since we're changing logo(s) anyway and since you're so good & fast in this, maybe we should try something different for both? And to continue parallel with music player, would it be hard for you to make another two:

  • one is just enlarged version of your circle with arrow (from your second logo);
  • second is the same but with "1" digit in the bottom left corner just like you did it with infinity (on your first logo).

Or am I too picky? Sorry to bother. But, as a compromise, how about this then:

  • keeping original inf logo for loop animation;
  • adding your 1+inf logo for one-time animation.

@discip
Copy link
Collaborator

discip commented Nov 22, 2023

This is the latest icon-set:

once

loop

Please let me know what you think about it.

@discip discip enabled auto-merge November 23, 2023 06:42
@ia
Copy link
Collaborator Author

ia commented Nov 23, 2023

Okay, it seems we're done here. But let me know if something must be changed and/or updated on my side.

P.S. I really do sincerely enjoy teamwork with you every time, people! 🙏

@discip discip requested a review from Ralim November 23, 2023 08:31
Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this ❤️

@discip discip merged commit 9579b6a into Ralim:dev Nov 23, 2023
15 checks passed
@ia ia deleted the animation-loop branch November 24, 2023 17:21
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.

3 participants