Skip to content

Added stopwatch#231

Merged
JF002 merged 16 commits intoInfiniTimeOrg:developfrom
Panky-codes:feature/add-stop-watch
Mar 21, 2021
Merged

Added stopwatch#231
JF002 merged 16 commits intoInfiniTimeOrg:developfrom
Panky-codes:feature/add-stop-watch

Conversation

@Panky-codes
Copy link
Copy Markdown

@Panky-codes Panky-codes commented Mar 13, 2021

Addresses issue: #144
Added a stopwatch with all the necessary functions such as play, pause, lap, stop.

20210313_223626
20210313_223544

For now, I hijacked the position of the paddle app. Let me know where the stopwatch belong in the app list :)

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Mar 18, 2021

Looks awesome! Thanks for this PR and sorry for the delay. I'll try to review it soon ;)

{Symbols::paintbrush, Apps::Paint},
{Symbols::info, Apps::Notifications},
{Symbols::paddle, Apps::Paddle},
//TODO: Need to find the right place based on comments from JF
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can take the slot of the 'Meter' app. This app is a simple demo app I added as a placeholder when there was no other apps available in InfiniTime. Now, we can remove it :)

That way, we'll keep the game Paddle in place ;)

Comment thread src/displayapp/screens/StopWatch.h Outdated

namespace Pinetime::Applications::Screens {

enum class States { INIT, RUNNING, HALTED };
Copy link
Copy Markdown
Collaborator

@JF002 JF002 Mar 20, 2021

Choose a reason for hiding this comment

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

Other enum classes do not name their values with CAPITAL letters. Could you rename these as 'Init', 'Running' and 'Halted' for more consistency?
Same for Events.

Comment thread src/displayapp/screens/StopWatch.h Outdated
}

private:
std::array<TimeSeparated_t, N> _arr;
Copy link
Copy Markdown
Collaborator

@JF002 JF002 Mar 20, 2021

Choose a reason for hiding this comment

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

Also for more consistency with the rest of the code : could you rename _arr as array for example? And maybe also currentSz to currentSize?

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Mar 20, 2021

I've just tested and reviewed your PR : it works as expected, and the code is pretty straightforward.
For the position on the menu, you can replace the Meter app by this new Stopwatch.
Not a big deal, but I've also added some comments about the naming of the variables.

Thanks!

@Panky-codes
Copy link
Copy Markdown
Author

Thanks for your comments. I made all the changes and also tested it once.

Just a note, I did a git pull of your develop branch and now I also see the lvgl submodules have been updated (src/libs/lvgl) in my PR. I didn't expect them but could you verify if it is ok or if I messed something up when I pulled your changes?

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Mar 21, 2021

Thanks for the fixes!
I'm not sure I understand what is going on with the submodule and why it was updated in your PR. I'll try to understand that before merging and potentially breaking something...

@Panky-codes
Copy link
Copy Markdown
Author

Hi JF,
Apparently, git pull doesn't really update the submodule. I went into the submodule and updated it to point to the same commit as your branch. And now there is no diff anymore for lvgl libs.

I think now it is ready to be merged. :)

Comment thread src/displayapp/DisplayApp.cpp Outdated
case Apps::Twos: currentScreen.reset(new Screens::Twos(this)); break;
case Apps::Paint: currentScreen.reset(new Screens::InfiniPaint(this, lvgl)); break;
case Apps::Paddle: currentScreen.reset(new Screens::Paddle(this, lvgl)); break;
//TODO: Change it back
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I just realized I forgot to revert this. I Will do it now.

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Mar 21, 2021

Apparently, git pull doesn't really update the submodule. I went into the submodule and updated it to point to the same commit as your branch. And now there is no diff anymore for lvgl libs.

I think you have to do a git module update after the pull to ensure that your submodule is up to date with the one from the repo.
There is no changes for the submodule in your PR so I guess that's good now :)

@Panky-codes
Copy link
Copy Markdown
Author

I did do the git submodule update, but somehow nothing changed. Anyway, I think everything is in order now and I also tested the paddle app so that I didn't introduce any regression.

I think now the PR is definitely ready to be merged :)

@JF002 JF002 merged commit 7b39130 into InfiniTimeOrg:develop Mar 21, 2021
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.

2 participants