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

Calendar app #923

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Calendar app #923

wants to merge 14 commits into from

Conversation

thnikk
Copy link

@thnikk thnikk commented Jan 9, 2022

This is a basic app using lvgl's calendar widget to show the current month with the day highlighted:

IMG_20220108_082419-01

There are currently 2 issues:

  1. It crashes on certain inputs (it seems like double taps and swipes.)
  2. Days of prevoius/next month aren't grayed out.

I'd also like to mention that I put this to the right of the clock, so it's accessible with a left swipe. I think this makes sense for the PineTimeStyle watch face, as the calendar is on the right side of the screen, but may make less sense in other situations. This is my first PR and I didn't want to submit it without fixing any outstanding issues, but debugging is very time consuming for me as I don't have a dev kit, so any help would be greatly appreciated.

@thnikk
Copy link
Author

thnikk commented Jan 9, 2022

I spent some more time with this and managed to fix the crashing by disabling clicks for the widget with lv_obj_set_click. I also added a status bar for more consistency. I'd still like to gray out the days from the previous and next month, but it's usable in its current state.

IMG_20220109_055642-01

@thnikk thnikk marked this pull request as ready for review January 9, 2022 14:07
@SteveAmor
Copy link
Contributor

Hi @thnikk this looks like a great little app. We're you going to add navigation to go forward and back months?
For greying out the forward and back month dates, I found lvgl/lvgl#1732

@thnikk
Copy link
Author

thnikk commented Jan 10, 2022

Thank you @SteveAmor for the tip, that's exactly what I needed. Days from inactive months are now grayed out and I've added navigation between months with up and down swipe gestures.
IMG_20220110_060920-01

@thnikk thnikk mentioned this pull request Jan 10, 2022
1 task
@dvidebaek
Copy link

dvidebaek commented Jan 11, 2022

Could you add week numbers? I’m a teacher an we use week numbers instead of dates. In that week etc.
Great work by the way!

maybe add a extra row that displays the week number. The first row should display the week number
Or maybe add the week number to the same line as year and month?

@Akselmo
Copy link
Contributor

Akselmo commented Jan 11, 2022

Pretty much what I was looking for! But week numbers would be nice :)

@medeyko
Copy link
Contributor

medeyko commented Jan 12, 2022

Thanks, a great app!

However, I would like to ask for several improvements:

  1. The today date should have a border or an inverted background (it is important for color blind persons). As far as I see, LVGL supports a thicker border around the today date; I however do not understand if it's possible to put a border for the today date, and put no borders for other dates.
  2. I'd prefer to change today date's color to LV_COLOR_GREEN, because red is associated with holidays/weekend.
  3. For the sake of gesture consistency, swipe up should close the app (Improve gesture consistency across the firmware #720). So I'd recommend to implement close by swipe up, and to use swipe left/swipe right for previous/next months.
  4. In many countries a week starts not with Sunday, but with Monday. It would be great to have a configuration option.
  5. Sundays and Saturdays should have LV_COLOR_RED color as weekends (it is traditional for calendars). Probably via lv_calendar_set_highlighted_dates.
  6. Also it may be comfortable to have a drop-down year/month to check any remote date. I see lv_calendar_header_dropdown_create(calendar) in the LVGL, wouldn't this help?

@dyamon
Copy link
Contributor

dyamon commented Jan 13, 2022

I would avoid using colors to denote weekends. If, in the long run, this is going to show calendar events as well, calendars use color labels to denote different categories of events. Agree on inverted fg/bg for today.

@Avamander
Copy link
Collaborator

Weekend kinda also has a very specific and constant location in a calendar as well 😛

@minacode
Copy link
Contributor

Maybe there can be a setting to let the weeks start on mondays.

@thnikk
Copy link
Author

thnikk commented Jan 14, 2022

@medeyko

1+2) That's a good point that I didn't consider. I picked red because I thought it stuck out well, but I would like for it to stick out for everyone. I don't thing green provides as good of contrast, but an inverted background looks fine.

  1. The up and down gestures are due to the placement of the app being to the right of the clock. I'd like some more input on this, as I'd like to not impose my prefernce for the placement of the app. The gestures are trivial enough to change to left and right for a normal standalone app, so it's not a big deal either way. I do think it may make sense to implement a week overview like what was shown off in WIP: Calendar / Timeline app and service #790, which could be accessed with another left swipe from the calendar.

  2. This can be changed in lv_conf.h with a define, but I'm not sure if there's any way to do this at runtime.

  3. I think this would only make sense for the day labels (though I don't believe it's possible,) but would clash with any future integration with calendar events. I agree with @dyamon and I think it makes sense to color the text for events and use an inverted background for the current day.

  4. This seems to just not be included with the library. It's possible to make an independent dropdown menu for month and year separate from the widget, but I don't believe there's a way to remove the header without just trying to hide it with styling, which I think is too hacky.

Week numbers could be included, but I'm not sure it's worth shrinking the calendar for. I think this may also make more sense in a week overview.

@levimega
Copy link

Does this work on pinetime if so I'm getting one

@medeyko
Copy link
Contributor

medeyko commented Feb 5, 2022

@thnikk

This can be changed in lv_conf.h with a define, but I'm not sure if there's any way to do this at runtime.

I looked in the LVGL sources. Definitely, it is not supposed to be changed at runtime. Alas!

@Lillian-Violet
Copy link

@thnikk

This can be changed in lv_conf.h with a define, but I'm not sure if there's any way to do this at runtime.

I looked in the LVGL sources. Definitely, it is not supposed to be changed at runtime. Alas!

Given that population wise Monday is the more common date to start the week with in the world, and it's the international standard in ISO 8601, I would recommend making the compile default first day of the week Monday and not Sunday, or you could offer two binaries for the different settings I suppose.

@medeyko
Copy link
Contributor

medeyko commented Apr 14, 2022

I guess, it's not a critical problem if the first day of a week is Monday or Sunday.
This minimalistic calendar would be already quite useful, there's no need to wait for other features, I think.

Are there any other issues blocking inclusion of the calendar into the main branch?

@thnikk
Copy link
Author

thnikk commented Apr 15, 2022

Just the position, It's currently accessed with a left swipe from the main clock screen. It could make it a normal app if that works better, I just put it to the right of the clock for convenience.

@Riksu9000
Copy link
Contributor

I think it should be made a normal app. Putting something on the right side of the watchface would be a significant design decision.

@toastom
Copy link

toastom commented Apr 15, 2022

I would love to see this calendar app included in InfiniTIme. If you could make it a normal app that would be great

@thnikk
Copy link
Author

thnikk commented Apr 16, 2022

Alright, it's now a normal app on a new app screen with an appropriate icon. I also changed the swipe directions from up and down to left and right so down swiping to close works as expected.

@thnikk
Copy link
Author

thnikk commented Apr 20, 2022

Would it be better to remove the statusbar altogether? There's not much else to do with the vertical space but enlarge or center the widget, but none of the other apps have one.

@ItzSwirlz
Copy link

Any update on this?

@medeyko
Copy link
Contributor

medeyko commented Jul 27, 2022

I would like to ask about it too: could anyone please write what is the state of this app?
Is there anything left to do with it before inlcusion?
How much resources does it really need? It uses specific calendar function in LVGL that may take some bytes.
How much resources are relly left?
Thanks!

@@ -435,6 +436,9 @@ void DisplayApp::LoadApp(Apps app, DisplayApp::FullRefreshDirections direction)
case Apps::Twos:
currentScreen = std::make_unique<Screens::Twos>(this);
break;
case Apps::Calendar:
currentScreen = std::make_unique<Screens::Calendar>(this, batteryController,dateTimeController);
Copy link
Contributor

@minacode minacode Dec 7, 2022

Choose a reason for hiding this comment

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

Suggested change
currentScreen = std::make_unique<Screens::Calendar>(this, batteryController,dateTimeController);
currentScreen = std::make_unique<Screens::Calendar>(this, batteryController, dateTimeController);

Copy link
Contributor

@minacode minacode left a comment

Choose a reason for hiding this comment

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

I am willing to help with this one, if anyone can tell what is left to do. Are there any known issues?

@NeroBurner
Copy link
Contributor

not a full list, just a few things that stick out for me:

  • rebase on current develop to get rid of the merge conflicts
  • firmware size comparison before and after adding the calendar

@Boteium
Copy link

Boteium commented Dec 7, 2022

This add 4kb to the final firmware.

}
lv_calendar_set_showed_date(calendar, &current);
return true;
}
Copy link

Choose a reason for hiding this comment

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

Let's also utilize SwipeUP/SwipeDown to switch to next/prev year.

Btw, a rebase is needed. And I've just done that so you call pull from:
https://github.com/Boteium/InfiniTime/tree/rebase_calendar

@JustScott
Copy link
Contributor

@Boteium 's branch fell out of date and was failing to build with the most recent 1.14 release, so I merged it with the most recent commit (ecf2f56) and updated the code so the build could succeed. As you'll see in the commit message I also commented out the calendar swipe up and down feature in src/displayapp/screens/Calendar.cpp as it interferes with the native swipe down to exit ability, but feel free to uncomment if you'd like this feature included.

I'm sure this isn't the best way to share the updated code in this PR, but I didn't want to open a new PR and clutter the repo... so I'll just give the link to my branch: https://github.com/JustScott/InfiniTime/tree/merge_calendar_upstream.

Also, I've only tested the pinetime-mcuboot-app version on my sealed pinetime. It works perfectly so i suspect the other versions will work as well.

@Boteium
Copy link

Boteium commented Jan 10, 2024

@JustScott at this point, maybe open another PR isn't a bad idea after all.

@FintasticMan
Copy link
Member

You can use git reset, git rebase and git push --force to update this pull request without having to make a new one.

@JustScott
Copy link
Contributor

JustScott commented Jan 10, 2024

Its owned by @thnikk, so they'll have to take my changes and update the pull request (I think that's how it works) if they want to. Otherwise I think how it is right now works fine as anyone who wants to use this app can just merge from my branch.

(mostly unrelated to this pull request) I've started to see that upstream Infinitime is less about adding new apps, and more about creating a very stable and minimal starting point for people to add their own apps, so odds are this may never get merged into the main branch, and that's fine, we just have to keep the app PRs we care about up to date. (sorry for the random epiphany, but it would've helped me to see this when I first got my pinetime)

@FintasticMan
Copy link
Member

FintasticMan commented Jan 10, 2024

Ah, didn't see that you aren't the one who made the PR :p. In that case it would be a good idea to open a new PR.

Now that we are able to decide which apps to include at compile-time, we're going to be a lot more happy to merge apps, and then not have them enabled by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new app This thread is about a new app
Projects
None yet
Development

Successfully merging this pull request may close these issues.