Sliding sidebar

[jhert0]

Problems that need to be fixed:

• Multiple threads are started after a double click
• The sidebar no longer responds to clicks

---

This change is 

[robinlinden]

The code overall looks good with a few nits. Test doesn't link on OS X because force_redraw isn't a thing on that platform right now.

---

Reviewed 6 of 11 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/chrono.c, line 25 at r2 (raw file):

        *info->ptr += info->step;
        yieldcpu(info->interval_ms);
        force_redraw();

Should chrono really be doing UI stuff? Isn't this the job of the callback function if it's needed?

---

src/devices.h, line 12 at r2 (raw file):

#ifndef ENABLE_MULTIDEVICE
// typedef uint8_t TOX_DEVICE_STATUS;

You can delete this.

---

src/ui.c, line 49 at r2 (raw file):

static bool background_mdbl(PANEL *UNUSED(p), bool triclick) {
    LOG_ERR("UI root", "bg dblclick %u", triclick);

Remove this.

---

src/ui.c, line 53 at r2 (raw file):

    int step = 0;

    if (!sidebar_chrono) { //sidebar_chrono has not been initialzed

Comment isn't needed because of the LOG_.

---

src/layout/sidebar.h, line 4 at r2 (raw file):

#define LAYOUT_SIDEBAR_H

#include <stdbool.h>

This file doesn't use bool.

---

Comments from Reviewable

[robinlinden]

Oh, and (optionally) the commit history could do with some cleanup if you feel like it.

[jhert0]

Review status: 6 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

src/chrono.c, line 25 at r2 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Should chrono really be doing UI stuff? Isn't this the job of the callback function if it's needed?

I changed it to use chrono_callback which is yieldcpu with a callback.

---

src/ui.c, line 53 at r2 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Comment isn't needed because of the LOG_.

Done.

---

src/devices.h, line 12 at r2 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

You can delete this.

Done.

---

src/layout/sidebar.h, line 4 at r2 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This file doesn't use bool.

Done.

---

Comments from Reviewable

[robinlinden]

Reviewed 11 of 11 files at r3, 4 of 5 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

src/ui.c, line 638 at r4 (raw file):

        case PANEL_NONE: {
            draw = background_mdbl(p, triclick);
            return draw;

Why was this change made? Do PANEL_NONE never have subpanels?

---

Comments from Reviewable

[jhert0]

Review status: 14 of 15 files reviewed at latest revision, 2 unresolved discussions.

---

src/ui.c, line 49 at r2 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Remove this.

Done.

---

src/ui.c, line 638 at r4 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Why was this change made? Do PANEL_NONE never have subpanels?

I did this because it wasn't working everytime i tried to get the sidebar to slide but i just reverted it and tried and it seemed to be working everytime. I'll revert this.

---

Comments from Reviewable

[robinlinden]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[robinlinden]

Reviewed 1 of 11 files at r3, 10 of 10 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

CMakeLists.txt, line 248 at r6 (raw file):

if (ENABLE_ASAN)
    if (UTOX_STATIC)
        message("ASAN is incompatible with static compilation, on some systems.")

This change doesn't belong in this PR.

---

src/flist.c, line 178 at r6 (raw file):

                    flist_draw_status_icon(status, w - SCALE(15), y + box_height / 2, f->unread_msg);
                }

Kill this line.

---

src/layout/settings.c, line 701 at r6 (raw file):

}

#include "../chrono.h"

It doesn't look like any changes in this PR would affect which headers this file needs.

---

Comments from Reviewable

[jhert0]

Review status: 12 of 15 files reviewed at latest revision, 3 unresolved discussions.

---

CMakeLists.txt, line 248 at r6 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This change doesn't belong in this PR.

Done.

---

src/flist.c, line 178 at r6 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Kill this line.

Done.

---

src/layout/settings.c, line 701 at r6 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

It doesn't look like any changes in this PR would affect which headers this file needs.

Done.

---

Comments from Reviewable

[robinlinden]

Reviewed 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/layout/settings.c, line 701 at r6 (raw file):

Previously, endoffile78 (Endoffile) wrote…

Done.

Is the other header added required? Because there are no changes to this file aside from adding the two headers.

---

Comments from Reviewable

[jhert0]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/layout/settings.c, line 701 at r6 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Is the other header added required? Because there are no changes to this file aside from adding the two headers.

It doesn't look like it's needed I don't know why it was added. I removed it.

---

Comments from Reviewable

[redmanmale]

Reviewed 5 of 11 files at r3, 7 of 10 files at r6, 2 of 3 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/flist.c, line 171 at r8 (raw file):

            }

            if (w > 100) {

Maybe introduce some constants for this one and one below?

---

Comments from Reviewable

[robinlinden]

I tested this on Windows for a bit:
Graphics explode if I toggle the sidebar: https://imgur.com/a/1EGXE
Segv occasionally when double clicking: https://gist.github.com/robinlinden/93573639d4be254798376b242a53e45b

[GrayHatter]

Reviewed 1 of 11 files at r1, 1 of 11 files at r3, 1 of 5 files at r4.
Review status: 14 of 15 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

src/ui.c, line 32 at r4 (raw file):

#include "ui/tooltip.h"

static CHRONO_INFO *sidebar_chrono = NULL;

This scope should be reduced to inside background_mdbl().

---

src/windows/drawing.c, line 17 at r9 (raw file):

#define MAX_BITMAPS BM_ENDMARKER + 1;

void *bitmap[MAX_BITMAPS];

= {0};

---

src/windows/drawing.c, line 27 at r9 (raw file):

void drawalpha(int bm, int x, int y, int width, int height, uint32_t color) {
    if (bm > MAX_BITMAPS) {

why isn't this >= BM_ENDMARKER.

IIRC endmarker isn't a SVG we're creating; thus, we would never need to draw endmarker?

See also why we're using END_MARKER +1 we shouldn't need to add 1 to the dummy enum that signals the end number of the enumerated list.

---

Comments from Reviewable

[GrayHatter]

Review status: 14 of 15 files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

src/windows/drawing.c, line 53 at r9 (raw file):

    // create temporary bitmap we'll combine the alpha and colors on
    uint32_t *out_pixel;

windows is fucky, set the value == NULL

---

src/windows/drawing.c, line 54 at r9 (raw file):

    // create temporary bitmap we'll combine the alpha and colors on
    uint32_t *out_pixel;
    HBITMAP   temp = CreateDIBSection(curr->mem_DC, &bmi, DIB_RGB_COLORS, (void **)&out_pixel, NULL, 0);

does this need to be void?

---

Comments from Reviewable

[jhert0]

Review status: 13 of 16 files reviewed at latest revision, 6 unresolved discussions.

---

src/ui.c, line 32 at r4 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

This scope should be reduced to inside background_mdbl().

I made it global so memory does'nt have to be allocated everytime the user double clicks.

---

src/windows/drawing.c, line 17 at r9 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

= {0};

Done.

---

src/windows/drawing.c, line 27 at r9 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

why isn't this >= BM_ENDMARKER.

IIRC endmarker isn't a SVG we're creating; thus, we would never need to draw endmarker?

See also why we're using END_MARKER +1 we shouldn't need to add 1 to the dummy enum that signals the end number of the enumerated list.

Fixed. I don't know why the + 1 is there, BM_ENDMARKER is just the number of bitmaps. It doesn't look necessary but maybe its something weird with windows? I can remove it if you want.

---

src/windows/drawing.c, line 53 at r9 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

windows is fucky, set the value == NULL

Done.

---

src/windows/drawing.c, line 54 at r9 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

does this need to be void?

The function is expecting void **'. I'm guessing the cast is there more for suppressing a warning than because it has to be.

---

Comments from Reviewable

[felixsanz]

even if hidden, if you click on that place where the button was supposed to be, the settings menu still brings up.