search

InfiniTimeOrg / InfiniTime

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

feat(Notifications): Adjust alert layout to show more text #1422

Open FrazerClews opened 2 years ago

FrazerClews commented 2 years ago

Original: https://github.com/InfiniTimeOrg/InfiniTime/pull/1348

To make better use of the space available in the header, it makes sense to increase the width of the alert_type.

To avoid confusion with the alert_counter, a ｜ will separate the alert_counter from the alert_type.

I do have a branch ready which furthers this PR by making better use of the height of the header space as there is a lot of space. This allows more text in the subject content, but this may cause accessibility issues.

before after

FrazerClews commented 2 years ago

@NeroBurner

nice improvement with minimal change, I like it

Improving the scroll speed should be done in a new PR to keep this one minimal

The firmware build is failing because of the branch name containing a / character, the PR itself seems fine

Thanks, I would also prefer to increase the scrolling speed, but not sure if it would be too fast for some people. I am happy to increase it in this PR though.

FrazerClews commented 2 years ago

@NeroBurner

nice improvement with minimal change, I like it Improving the scroll speed should be done in a new PR to keep this one minimal The firmware build is failing because of the branch name containing a / character, the PR itself seems fine

Thanks, I would also prefer to increase the scrolling speed, but not sure if it would be too fast for some people. I am happy to increase it in this PR though.

After doing some tests, I remembered experimenting with this a while back. One problem I found was that by the time notification loads, a good portion of the text at the beginning has rotated and unreadable.

I tried looking for a way to maybe pause the alert text for a second then start the the circular rotation, but there is no built functionality in LVGL 7.11 for that from what I have seen.

NeroBurner commented 2 years ago

Nice, now the CI is all green :heavy_check_mark:

I'm not against the scroll speed improvement, but I advice to do it in a different PR to keep this one minimal and easy to review. By mashing different improvements into one PR the PR gets bigger, harder to review. And more people with differing opinions, which could lead to longer time to reach a consensus, blocking the other improvements to be merged. So small PRs improving one thing are a good thing in my opinion :)

FrazerClews commented 2 years ago

Sorry, not too sure why I read your previous message as including animation speed up in this PR, but I agree to keep them separate.

JF002 commented 1 year ago

The additional space for the notification title is more than welcome. However, I'm not completely satisfied by the | separator... I don't know how to explain it, but it doesn't feel right. Maybe we could experiment with LVGL object mask to some kind of shading effect at the end of the label, for example?