-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Music: Redesign Part 2 #2337
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
base: main
Are you sure you want to change the base?
Music: Redesign Part 2 #2337
Conversation
Alarm: Simplify alarm alerting screen (InfiniTimeOrg#2211)
…oach to switching between skipping and volume buttons
|
Build checks have not completed. Possible reasons for this are:
|
|
@tituscmd Can I ask about the Track/Artist info (currently placeholder "Some Track")? Is the "Disconnected" replaced with the artist when connected? I liked the two line design from the first post, since it would reduce text scrolling. The current commit ed30e5 from https://github.com/InfiniBros/InfiniTime/tree/music_redesign_2 makes InfiniSim crash on my end when entering the music app. It happens reliably with the following callstack: The debugger (gdb) prints I can see that the issue is caused by |
Yes, sure! In the actual version. When your watch is not connected to any bluetooth device, it would just show "Disconnected" and the upper text would be blank. On another note, I can tell that we're going with the first option where the music buttons are up top, so I'll be pushing that in a bit. Once that's done, I'll also show a picture of both states on device and remove all the commented out code |
This seemed to have been caused by some 0 division on the progress bar. Should be fixed by latest commit - works for me in InfiniEmu now |
…matting commit (ironically one more)
|
Latest commit makes the UI switch on reconnection much smoother, since we now force fetch the data and update the UI if we just reconnected - otherwise we only fetch new data if it changed. |
|
Looking good, @tituscmd! Thanks for your work on this! |
|
This PR should be ready for review from my side :) |
|
I'm of the belief that track info isn't needed on a watch or should be at least be minimized to a progress bar. The buttons could them be laid out in a way that would make them easier to press. Just my 2 cents. |
|
@0x0000ff That being said, I think it's more important at this point to get the UI merged so people can actually start using it when the next version of InfiniTime is released... More changes can always be made in the future. |
Not quite sure what you mean by this. Care to elaborate? |
Agreed. Should be all good to merge from my side I think. |
mark9064
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good :)
Haven't tested locally or ran clang-tidy yet
| "streambuf": "cpp", | ||
| "cinttypes": "cpp", | ||
| "typeinfo": "cpp" | ||
| "typeinfo": "cpp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this change be moved to another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally accidental - not even sure how it happened but VSC seems to do that sometimes. I'll remove it ASAP
|
|
||
| void Music::UpdateLength() { | ||
| int remaining = totalLength - currentPosition; | ||
| if (remaining < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add braces here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean
int remaining = (totalLength - currentPosition);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the if statements:
if (remaining < 0) {
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, makes sense. I'll do that as well as soon as I get the time to
| bleState = bleController.IsConnected(); | ||
| if (bleState.Get() == false) { | ||
| SetDisconnectedUI(); | ||
| lastConnected = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could lastConnected be avoided by relying on bleState.IsUpdated()?
| } | ||
| } | ||
|
|
||
| void Music::SetDisconnectedUI() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the state variables (playing, artist, track, album etc) were updated here, could we get rid of force? I think the UI will be more robust if the state variables are always kept up to date, that way connection/disconnection don't need special handling
Would also need handling during in the constructor
| static constexpr const char* sleep = "\xEE\xBD\x84"; | ||
| static constexpr const char* calculator = "\xEF\x87\xAC"; | ||
| static constexpr const char* backspace = "\xEF\x95\x9A"; | ||
| static constexpr const char* swap = "\xef\x81\xb9"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, unused symbol from the experimental "Swap Controls"-button. I'll remove it ASAP
|
|
||
| Pinetime::Controllers::MusicService& musicService; | ||
| const Controllers::Ble& bleController; | ||
| Pinetime::Controllers::DateTime& dateTimeController; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time controller unused?
| const Controllers::Ble& bleController; | ||
| Pinetime::Controllers::DateTime& dateTimeController; | ||
|
|
||
| std::string artist; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these could all use DirtyValue
Co-authored-by: mark9064 <[email protected]>






Hey everybody,
Since creating PR #2292 and introducing a small little redesign of the music app, I've had some more time on my hands and finally came around to creating a better version of it.
I've moved the artist and track name up a bit to add a new progress bar that shows the progress of the song you're listening to, with the time that has passed shown on the left and the time remaining shown on the right.
Sadly, I've had to remove (comment out) the disc animation in the top right because of space issues. If anyone is especially fond of the disc I'm sure space can be made to fit it back in. It is definitely not a final decision by me, hence why it's only commented out and not removed altogether.
But now that I've temporarily removed the disc animation, the top of the screen seemed quite empty to me. And after some thinking and digging through the code, I stumbled upon these lines of code in the Music.cpp:
And so I thought "why not do that?" and I added an indicator text at the top telling you if you are connected via bluetooth. Please share your opinion on this.
Finally, here are some pictures of it all:

