Idle status

[redmanmale]

aka auto away (#325) aka presence state (#167).

After some configurable time of inactivity (no mousemoves or keystrokes) your status will be automatically changed to away. In case of activity it will be changed back to online.

• save settings
• UI: switch for on/off, label and input for value
• Windows: timer to check for last user activity
• Linux: select() and check for last user activity via x11 screensaver extensions libxss-dev (it's used everywhere)
• macOS: timer to check for last user activity (implemented by @publicarray)

---

This change is 

[redmanmale]

@publicarray
Could you help me with macOS part?

[publicarray]

@redmanmale Yea I'll look into it. Do you want me to add commits to this pr?

[publicarray]

There is a race condition when the status has switched to idle (due to inactivity) and the user quickly (before the code switches the status) presses the status button to become online. The idle code will no longer switch to idle status automatically.

[redmanmale]

Thank you very much @publicarray!

[publicarray]

You are very welcome 😄 Love to help.

[publicarray]

This is just a guess but I don't think !PLATFORM_ANDROID is needed. Isn't the file only included on MacOS? Thanks for fixing my spelling!

[redmanmale]

Yes, it's a little overkill, but I want to use the same condition !DISABLE_IDLE_STATUS && !PLATFORM_ANDROID everywhere.

[publicarray]

IMHO I prefer to keep platform specific stuff tied to that platform and not sprinkle it over to other platforms.

[GrayHatter]

I'll re-review once there's not an #ifdef in every file.

+@GrayHatter

---

Reviewed 3 of 11 files at r1.
Review status: 3 of 12 files reviewed at latest revision, 4 unresolved discussions.

---

langs/en.h, line 495 at r3 (raw file):

 * Status name (Away) should be the same as STATUS_AWAY
 */
#if !DISABLE_IDLE_STATUS && !PLATFORM_ANDROID

don't add an #if here

---

langs/i18n_decls.h, line 169 at r3 (raw file):

    STR_SETTINGS_UI_AUTO_HIDE_SIDEBAR,

    #if !DISABLE_IDLE_STATUS && !PLATFORM_ANDROID

these should always exist. Even if they're not used no a platform...

poor android...

---

langs/ru.h, line 416 at r3 (raw file):

 * Название статуса (не на месте) должно быть таким же, как STATUS_AWAY
 */
#if !DISABLE_IDLE_STATUS && !PLATFORM_ANDROID

no #if

---

src/settings.c, line 73 at r3 (raw file):

    .magic_flist_enabled    = false,

    #if !DISABLE_IDLE_STATUS && !PLATFORM_ANDROID

I'm gonna stop commenting but I don't like #if's unless they're features you have a strong reason to compile in/out. X11 must exist, so Xss is a fine dependency to add.

If you really want to be pedantic, you could detect for the existence of Xss, and then just disable the option via the UI

---

Comments from Reviewable

[redmanmale]

@GrayHatter
Can I just drop DISABLE_IDLE_STATUS option and always require X11 package?

[redmanmale]

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions.

---

langs/en.h, line 495 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

don't add an #if here

Done.

---

langs/i18n_decls.h, line 169 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

these should always exist. Even if they're not used no a platform...

poor android...

Done.

---

langs/ru.h, line 416 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

no #if

Done.

---

src/settings.c, line 73 at r3 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

I'm gonna stop commenting but I don't like #if's unless they're features you have a strong reason to compile in/out. X11 must exist, so Xss is a fine dependency to add.

If you really want to be pedantic, you could detect for the existence of Xss, and then just disable the option via the UI

Done.

---

Comments from Reviewable

[jhert0]

---

Reviewed 12 of 12 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[GrayHatter]

@redmanmale I think you have dropped DISABLE_IDLE_STATUS, which is fine. But this is going to take me some time to review. I'm not convinced XNextEventTimed is the best way to go about this, so I'll have to do a little research.

If you want to merge this sooner, you can wrap the xlib stuff in an #ifdef 0 and comments as to why, then I can just review the Windows stuff which on first glance looks fine.

---

Reviewed 2 of 12 files at r4, 2 of 6 files at r5.
Review status: 8 of 12 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[redmanmale]

I don't like to merge unfinished features so I'll wait for your review.
But I hope we'll have this in next release (0.17.0), otherwise I'd prefer to comment out Xlib part as you said.

---

Review status: 8 of 12 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[robinlinden]

Reviewed 6 of 12 files at r4, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

CMakeLists.txt, line 301 at r5 (raw file):

set(LIBRARIES ${LIBRARIES} ${LIBSODIUM_LIBRARIES})

if(UNIX AND NOT APPLE)

@endoffile78 Can you test this on whateverBSD?

---

langs/en.h, line 499 at r5 (raw file):

 */
msgid(SETTINGS_IDLE_STATUS)
msgstr("Show me as Away after")

You should probably combine this string with the one below and add a %u format specifier for the number to make localisation less of a chore.

---

src/settings.c, line 73 at r5 (raw file):

    .magic_flist_enabled    = false,

    // TODO: Add to save

Can this be done now that the .ini PR is merged?

---

src/cocoa/main.m, line 41 at r5 (raw file):

#define DEFAULT_HEIGHT (320 * DEFAULT_SCALE)

struct utox_self *tox_self; // reference to self from self.h

Make this static.

---

src/layout/settings.c, line 1419 at r5 (raw file):

static void edit_idle_interval_onenter(EDIT *UNUSED(edit)) {
    edit_idle_interval.data[edit_idle_interval.length] = 0;
    settings.idle_interval = strtol((char *)edit_idle_interval.data, NULL, 0);

What happens when settings.idle_interval is set to 0? (If strtol fails.) Should we do some sanity/bound checking here?

---

src/windows/main.c, line 813 at r5 (raw file):

static void CALLBACK idle_handler(HWND UNUSED(hwnd), UINT UNUSED(message), UINT UNUSED(idTimer), DWORD dwTime)
{

Move this to the line above. (1TBS)

---

src/windows/main.c, line 823 at r5 (raw file):

    GetLastInputInfo(&last_active);

    if (dwTime - last_active.dwTime > (uint16_t)(settings.idle_interval * 1000 * 60)) {

This should probably not be cast to an uint16_t since 2 seconds would mean that the number we're casting is 120 000 and a bit bigger than what an uint16 can contain.

---

src/xlib/main.c, line 753 at r5 (raw file):

            tv->tv_usec = 0;
            return false;
        } else {

You can let this fall through to the end of the function where this code also exists.

---

src/xlib/main.c, line 757 at r5 (raw file):

            return true;
        }
    } else {

No else after return.

---

Comments from Reviewable

[GrayHatter]

bump

[redmanmale]

Review status: 0 of 12 files reviewed at latest revision, 9 unresolved discussions.

---

langs/en.h, line 499 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

You should probably combine this string with the one below and add a %u format specifier for the number to make localisation less of a chore.

I can't do it just like that because it used in the different UI labels.

---

src/settings.c, line 73 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Can this be done now that the .ini PR is merged?

Done.

---

src/cocoa/main.m, line 41 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Make this static.

Done.

---

src/layout/settings.c, line 1419 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

What happens when settings.idle_interval is set to 0? (If strtol fails.) Should we do some sanity/bound checking here?

Done.

---

src/windows/main.c, line 813 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Move this to the line above. (1TBS)

Done.

---

src/windows/main.c, line 823 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably not be cast to an uint16_t since 2 seconds would mean that the number we're casting is 120 000 and a bit bigger than what an uint16 can contain.

Done.

---

src/xlib/main.c, line 753 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

You can let this fall through to the end of the function where this code also exists.

Done.

---

src/xlib/main.c, line 757 at r5 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

No else after return.

Done.

---

Comments from Reviewable

[redmanmale]

It's been a while... Can we merge this?

[GrayHatter]

Robin is away on vacation, he'll have to take a look when he gets back.

---

Reviewed 2 of 12 files at r6.
Review status: 12 files need 2 more reviewers, 9 open discussions remaining, 1 assignee LGTMs are missing, missing Admin LGTM at current revision. (waiting on @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, and @undefined)

---

Comments from Reviewable

[GrayHatter]

Reviewed 2 of 8 files at r7.
Review status: 12 files need 2 more reviewers, 10 open discussions remaining, 1 assignee LGTMs are missing, missing Admin LGTM at current revision. (waiting on @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, and @undefined)

---

src/ui.c, line 147 at r7 (raw file):

    CREATE_SWITCH(auto_startup,      10, 150, _BM_SWITCH_WIDTH, _BM_SWITCH_HEIGHT);
    CREATE_SWITCH(mini_contacts,     10, 180, _BM_SWITCH_WIDTH, _BM_SWITCH_HEIGHT);
    CREATE_EDIT(idle_interval, _BM_SWITCH_WIDTH + 25 + UN_SCALE(UTOX_STR_WIDTH(SETTINGS_IDLE_STATUS)), 211, 50, 24);

this isn't supposed to be in here anymore. It belongs in the layout dir

---

Comments from Reviewable

[GrayHatter]

Reviewed 1 of 12 files at r6, 2 of 8 files at r7.
Review status: 12 files need 2 more reviewers, 11 open discussions remaining, 1 assignee LGTMs are missing, missing Admin LGTM at current revision. (waiting on @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, and @undefined)

---

src/settings.h, line 20 at r7 (raw file):

 * Period (in seconds) for check is user idle or not.
 */
static const uint16_t idle_check_period = 1;

I defines in headers make me nervous... ESPECIALLY when you make them static as well.

Plus, wouldn't time_t be better here?

---

Comments from Reviewable

[redmanmale]

Review status: 12 files need 2 more reviewers, 11 open discussions remaining, 1 assignee LGTMs are missing, missing Admin LGTM at current revision. (waiting on @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, @undefined, and @undefined)

---

src/settings.h, line 20 at r7 (raw file):

I defines in headers make me nervous... ESPECIALLY when you make them static as well.

What do you mean? Is there a better place for it? Or something else?

wouldn't time_t be better here

On macOS it's double for seconds, on Win it's uint for milliseconds.
Only on Linux it's time_t.

---

src/ui.c, line 147 at r7 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

this isn't supposed to be in here anymore. It belongs in the layout dir

IIRC I tried but failed.
It couldn't be done because of the macros UN_SCALE and UTOX_STR_WIDTH, panel struct definition allows only constans.

---

Comments from Reviewable