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

messages: add option to not show the first unread message #3622

Merged

Conversation

bobrippling
Copy link
Collaborator

For example, if a user wants to keep them unread and just scroll through what's arrived

@gfwilliams
Copy link
Member

I think it's expected that the default for settings.showMsgIfUnread should be 1? It is in settings? But then in the messagegui app it's not set so the default is 0, so any user upgrading will suddenly not have the latest message shown.

It might make more sense to rename it to settings.ignoreUnread/similar so then you can simply check it with !! and we don't have to have defaults in all the files?

@bobrippling
Copy link
Collaborator Author

I think it's expected that the default for settings.showMsgIfUnread should be 1? It is in settings?

It is indeed:

if (settings.showMsgIfUnread === undefined) settings.showMsgIfUnread = 1;

But then in the messagegui app it's not set so the default is 0, so any user upgrading will suddenly not have the latest message shown.

Yes - there are other settings which are defaulted in messages but not in messagegui, so a new user will also experience issues too:

if (settings.vibrate===undefined) settings.vibrate=":";
if (settings.vibrateCalls===undefined) settings.vibrateCalls=":";
if (settings.repeat===undefined) settings.repeat=4;
if (settings.repeatCalls===undefined) settings.repeatCalls=settings.repeat;
if (settings.vibrateTimeout===undefined) settings.vibrateTimeout=60;
if (settings.unreadTimeout===undefined) settings.unreadTimeout=60;
if (settings.maxMessages===undefined) settings.maxMessages=3;
if (settings.iconColorMode === undefined) settings.iconColorMode = iconColorModes[0];

It might make more sense to rename it to settings.ignoreUnread/similar so then you can simply check it with !! and we don't have to have defaults in all the files?

Sure - although considering the above, do we perhaps want to either duplicate/share the defaulting to fix messages for new users, or would you be happy with the rename?

@gfwilliams
Copy link
Member

I think just rename for now.

It's a bit of a pain because we use the same settings file for several different apps, so for instance maxMessages is only used in the widget, but we do:

let settings = Object.assign({flash: true, maxMessages: 3}, require("Storage").readJSON("messages.settings.json", true) || {});

I just checked and as far as I can see all those things you mention actually do have defaults that are handled ok, they're just not done in a very nice way often (eg vibrateCalls/etc). I guess it might be nice to use the Object.assign pattern above at some point and clean the code up, but I think for now we're good

@bobrippling bobrippling force-pushed the feat/messages-no-show-1st-unread branch from 7508233 to 03b1519 Compare November 7, 2024 17:59
@bobrippling
Copy link
Collaborator Author

Thanks - rename done, ready for a re-review

Copy link
Member

@gfwilliams gfwilliams left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me! I'm happy to merge whenever you are.

One strange thing is the use of settings().stuff in the settings app which causes settings to be loaded multiple times just to show one menu. That was there already though :)

@bobrippling
Copy link
Collaborator Author

Thanks - yeah that's a fair point, shall merge this first though

@bobrippling bobrippling merged commit d3dc3b4 into espruino:master Nov 11, 2024
1 check passed
@bobrippling bobrippling deleted the feat/messages-no-show-1st-unread branch November 11, 2024 12:07
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