Notification age (x minutes ago) on notifcation screen #835

clemensvonmolo · 2021-11-17T14:18:06Z

This PR adds "x time ago" to the bottom of the notification screen. Every notification item now stores the time at which it was received as a std::time_t (saw no reason to use std::chrono::time_point as it takes more memory with no advantages for storage). The time arrived is set in the notification manager so existing apps don't have to change anything.

I'm not really sure about the location of the text (image below) and if it would be better to use C-style string operations for memory usage and allocation optimisation.

..e indicator does not interfere with the message

SteveAmor · 2021-11-17T19:38:35Z

Great feature.

Itai-Nelken · 2021-12-01T19:03:24Z

Works nicely. Only problem is that the 'no notifications' notification has it (and it doesn't seem to update).

src/displayapp/screens/Notifications.cpp

Tiggilyboo · 2022-05-14T16:11:27Z

Not sure if this is still active, but this would be great to get merged. What is left to do / blocking this from getting in?

clemensvonmolo · 2022-05-15T09:46:24Z

The problem seems to have been me forgetting about it. Now I think this has to be adjusted to newer changes that happened in the meantime. I don't have time today but I can try doing that tomorrow.

devnoname120 · 2022-06-18T19:51:40Z

The problem seems to have been me forgetting about it. Now I think this has to be adjusted to newer changes that happened in the meantime. I don't have time today but I can try doing that tomorrow.

Friendly bump 🙏

JF002 · 2022-06-19T19:31:35Z

@clemensvonmolo @devnoname120 Sorry for not providing any feedback about this PR for so long. Is there any reason for it to be still a draft?

clemensvonmolo · 2022-06-19T19:54:13Z

I can set it to a normal PR for now, but Github says there will be conflicts and I can't do anything about that in the next three days while I'm on a class trip. It might also be a good idea to discuss the positioning of the text again. Anyway thanks for the positive feedback and patience everyone!

clemensvonmolo · 2022-07-28T17:57:48Z

Okay this should work now. I've also created InfiniTimeOrg/InfiniSim#46 to fix the build issue it is currently facing with this PR.

Riksu9000

In a follow up PR there could be special messages like "Just now" or "Yesterday".

Riksu9000 · 2022-07-30T06:07:15Z

src/displayapp/screens/Notifications.cpp

+    } else {
+      timeUnit = 'm';
+    }
+    lv_obj_t* alert_age = lv_label_create(container, nullptr);


Variables names should be like this. Don't commit this suggestion.

Suggested change

lv_obj_t* alert_age = lv_label_create(container, nullptr);

lv_obj_t* alertAge = lv_label_create(container, nullptr);

Riksu9000 · 2022-07-30T06:08:41Z

src/displayapp/screens/Notifications.cpp

+    lv_obj_t* alert_age = lv_label_create(container, nullptr);
+    lv_label_set_text_fmt(alert_age, "%d%c ago", ageInt, timeUnit);
+    // same format as alert_count
+    lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, 0, -16);


This will make the padding similar to the text.

Suggested change

lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, 0, -16);

lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, -10, -10);

Riksu9000 · 2022-07-30T06:16:46Z

src/displayapp/screens/Notifications.cpp

+    if ((ageInt / (60 * 24)) >= 1) {
+      ageInt /= (60 * 24);
+      timeUnit = 'd';
+    } else if ((ageInt / 60) >= 1) {


These can be simplified.

Suggested change

if ((ageInt / (60 * 24)) >= 1) {

ageInt /= (60 * 24);

timeUnit = 'd';

} else if ((ageInt / 60) >= 1) {

if (ageInt >= 60 * 24) {

ageInt /= (60 * 24);

timeUnit = 'd';

} else if (ageInt >= 60) {

NeroBurner · 2022-10-12T20:48:32Z

src/components/ble/NotificationManager.h

@@ -57,6 +62,7 @@ namespace Pinetime {
      size_t NbNotifications() const;

    private:
+      Controllers::DateTime& dateTimeController;


we can even store a const reference, to make it explicit, that a notification can't change the date-time, just get the current date and time

Suggested change

Controllers::DateTime& dateTimeController;

const Controllers::DateTime& dateTimeController;

NeroBurner · 2022-10-12T20:48:48Z

src/components/ble/NotificationManager.h

        std::array<char, MessageSize + 1> message;
        Categories category = Categories::Unknown;

        const char* Message() const;
        const char* Title() const;
      };

+      NotificationManager(Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} {


we can even store a const reference, to make it explicit, that a notification can't change the date-time, just get the current date and time

Suggested change

NotificationManager(Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} {

NotificationManager(const Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} {

yusufmte · 2023-01-31T21:08:51Z

Any plans to rebase/eventually merge this sometime? Seems it would be a nice improvement.

LinuxinaBit · 2023-04-18T21:44:26Z

This would go very well with #1697 if the grey notification box was made slightly smaller vertically to allow a small bottom bar for this and the notification icons.

Boteium · 2023-07-21T14:16:30Z

lv_obj_t* alert_age should be the last object to be created. Otherwise, objects such as alert_caller will be on top of it and cause the alert_age to be invisible in IncomingCall message.

This also needs a rebase.

first try at adding notification age

8466196

Riksu9000 reviewed Dec 3, 2021

View reviewed changes

src/displayapp/screens/Notifications.cpp Outdated Show resolved Hide resolved

src/displayapp/screens/Notifications.cpp Outdated Show resolved Hide resolved

minor changes to notification age logic

1de1d44

clemensvonmolo marked this pull request as ready for review June 19, 2022 19:54

clemensvonmolo and others added 2 commits July 1, 2022 20:00

Merge branch 'develop' into notification-time

a7e8139

fix outdated constructor call for NotificationItem

afe96d5

clemensvonmolo added a commit to clemensvonmolo/InfiniSim that referenced this pull request Jul 28, 2022

match notificationManager to InfiniTimeOrg/InfiniTime#835

146949c

clemensvonmolo mentioned this pull request Jul 28, 2022

match notificationManager to InfiniTimeOrg/InfiniTime#835 InfiniTimeOrg/InfiniSim#46

Open

clemensvonmolo requested a review from Riksu9000 July 28, 2022 17:58

Riksu9000 added this to the 1.11.0 milestone Jul 30, 2022

Riksu9000 suggested changes Jul 30, 2022

View reviewed changes

NeroBurner added the feature request label Oct 12, 2022

NeroBurner requested changes Oct 12, 2022

View reviewed changes

NeroBurner modified the milestones: 1.11.0, 1.12.0 Oct 16, 2022

JF002 removed this from the 1.12.0 milestone Jan 5, 2023

LinuxinaBit mentioned this pull request Apr 18, 2023

Add Notification icon stack #1697

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification age (x minutes ago) on notifcation screen #835

Notification age (x minutes ago) on notifcation screen #835

clemensvonmolo commented Nov 17, 2021

SteveAmor commented Nov 17, 2021

Itai-Nelken commented Dec 1, 2021

Tiggilyboo commented May 14, 2022

clemensvonmolo commented May 15, 2022

devnoname120 commented Jun 18, 2022

JF002 commented Jun 19, 2022

clemensvonmolo commented Jun 19, 2022

clemensvonmolo commented Jul 28, 2022

Riksu9000 left a comment

Riksu9000 Jul 30, 2022 •

edited

Loading

Riksu9000 Jul 30, 2022

Riksu9000 Jul 30, 2022

NeroBurner Oct 12, 2022

NeroBurner Oct 12, 2022

yusufmte commented Jan 31, 2023

LinuxinaBit commented Apr 18, 2023

Boteium commented Jul 21, 2023

	lv_obj_t* alert_age = lv_label_create(container, nullptr);
	lv_obj_t* alertAge = lv_label_create(container, nullptr);

	lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, 0, -16);
	lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, -10, -10);

	Controllers::DateTime& dateTimeController;
	const Controllers::DateTime& dateTimeController;

	NotificationManager(Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} {
	NotificationManager(const Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} {

Notification age (x minutes ago) on notifcation screen #835

Are you sure you want to change the base?

Notification age (x minutes ago) on notifcation screen #835

Conversation

clemensvonmolo commented Nov 17, 2021

SteveAmor commented Nov 17, 2021

Itai-Nelken commented Dec 1, 2021

Tiggilyboo commented May 14, 2022

clemensvonmolo commented May 15, 2022

devnoname120 commented Jun 18, 2022

JF002 commented Jun 19, 2022

clemensvonmolo commented Jun 19, 2022

clemensvonmolo commented Jul 28, 2022

Riksu9000 left a comment

Choose a reason for hiding this comment

Riksu9000 Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Riksu9000 Jul 30, 2022

Choose a reason for hiding this comment

Riksu9000 Jul 30, 2022

Choose a reason for hiding this comment

NeroBurner Oct 12, 2022

Choose a reason for hiding this comment

NeroBurner Oct 12, 2022

Choose a reason for hiding this comment

yusufmte commented Jan 31, 2023

LinuxinaBit commented Apr 18, 2023

Boteium commented Jul 21, 2023

Riksu9000 Jul 30, 2022 •

edited

Loading