search

espruino / BangleApps

Bangle.js App Loader (and Apps)
https://banglejs.com/apps
MIT License
496 stars 1.17k forks source link

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

Closed bobrippling closed 2 weeks ago

bobrippling commented 1 month ago

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

gfwilliams commented 3 weeks ago

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 commented 3 weeks ago

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

It is indeed: https://github.com/espruino/BangleApps/blob/f5b3ad281051e723bd99eeab7570de6a66fd5df1/apps/messages/settings.js#L14-L14

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:

https://github.com/espruino/BangleApps/blob/ebd2d5b7b9c654d538205b4b86b4691d38b536e1/apps/messages/settings.js#L6-L13

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 commented 3 weeks ago

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 commented 2 weeks ago

Thanks - rename done, ready for a re-review

bobrippling commented 2 weeks ago

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