InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.63k stars 902 forks source link

Up to date calendar app #1958

Open JustScott opened 5 months ago

JustScott commented 5 months ago

This calendar app was originally created by @thnikk in pull request #923, then was updated in June of 2023 by @Boteium to work with the most recent Infinitime version at that time. In the most recent version of Infinitime, 1.14 in January of 2024 (now), the way apps are handled has changed, so in this PR I've updated the code to work with those changes and to be able to be merged into version 1.14 of Infinitime. I've tested the pinetime-mcboot-app version on my sealed Pinetime and have yet to find and issue with it.

Also, I commented out the swipe up and down to increase and decrease the current calendar year feature as that interfered with the swipe down to exit ability users have come to expect when navigating apps... feel free to uncomment the code if you'd like that feature to stay.

github-actions[bot] commented 5 months ago

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed
JustScott commented 5 months ago

I just saw the line: 'PRs are either rebase or squash merged' in https://github.com/InfiniTimeOrg/InfiniTime/blob/ecf2f564f7a0884b6acdfdf5530abe2b98cb9aa9/doc/maintainer-guide.md#reviewing-prs.

I don't squash merges or rebase often so I'm not great at it, but I can try if this is required, just let me know!

vkareh commented 5 months ago

@JustScott the easiest way is to run git rebase -i HEAD~3 (where 3 is the number of commits that you want to squash).

In there it will give you instructions on how to use it:

  1. mark all but the first commit to be squashed
  2. write a proper commit message
  3. save and exit the editor
  4. run git push -f to update the PR

Note: You might have issues doing the above, since I think the branch you sourced this from has the actual calendar implementation buried deep in the git history. I would start over with a clean branch off of main and just cherry-pick the commit (or just copy/paste the code), then fix and squash.

JustScott commented 5 months ago

@vkareh Okay I reset this PRs (local) branch to the latest upstream commit, then cherry picked code from @thnikk's & @Boteium's branches onto my branch (which helped me avoid some of the uneeded 'legacy' code, thanks for the suggestion!).

Since I didn't bring along any of the prior commits from the other branches, and can just put all of my changes under a single commit now, doesn't that mean I don't need to do any squashing?

FintasticMan commented 5 months ago

Thanks for making this new PR. Could you attribute @thnikk and @Boteium in the commit? You can do that by adding Co-authored-by: <commit author> to the commit message.

vkareh commented 5 months ago

@JustScott

Since I didn't bring along any of the prior commits from the other branches, and can just put all of my changes under a single commit now, doesn't that mean I don't need to do any squashing?

That's correct. The point of squashing is to merge multiple commits, which is no longer necessary in your case.

I also agree with @FintasticMan about adding attribution. You can change the commit message using the same git rebase -i HEAD~1 command and choosing reword (or just r) as the command. Once you save and exit, it will open up the editor with the commit message to edit. Save again, exit, git push -f.

JustScott commented 5 months ago

@vkareh I believe I addressed all the concerns above with this last commit. I also tested this on my pinetime and everything is working as expected.

I'm also wondering what the thoughts on the ability to swipe up and down to increase and decrease the current year of the calendar are? I personally don't like this as it interferes with the user's ability swipe down to exit the app, but I'll leave it in if others want this capability.

vkareh commented 5 months ago

@JustScott

@vkareh I believe I addressed all the concerns above with this last commit. I also tested this on my pinetime and everything is working as expected.

Awesome! You can still squash the new commit you made if you want to clean things up further. Not sure if it's necessary though.

I'm also wondering what the thoughts on the ability to swipe up and down to increase and decrease the current year of the calendar are? I personally don't like this as it interferes with the user's ability swipe down to exit the app, but I'll leave it in if others want this capability.

I saw that and thought the same thing. It would certainly be useful, but interfering with the global swipe mechanics feels wrong. You would need to use the button to exit, so it's a tradeoff between fuctionality and consistency. I would leave it up to the repo maintainers to determine that.

JustScott commented 5 months ago

@vkareh I figured I'd wait until this was ready to be merged to the main branch to squash the commits so the maintainers can more clearly see what's changed. Also, I appreciate all the help you've given me with rebase commands above!

ZekeZDev commented 4 months ago

Very nice app, I am personally a fan of the original implementation from #923 in which the calendar was placed to the right of the clock. I have found as I add more applications to infinitime its really nice to be able to access the calendar so simply. I'd personally like it as a default, however it might be a better option for a future pull request to customize the location of the launcher, setting and notification gestures too.

https://github.com/InfiniTimeOrg/InfiniTime/assets/81902668/7b72d291-21ec-4a83-87c3-583d71ca2f88

I find it also feels a bit more natural for the vertical swiping with this setup as its closer to a traditional calendar.