Topic: tabpanel.cc use of TAB key

The current code in handle_key(…) is:

uint32_t selected_idx = active();
const uint32_t max = tabs_.size() - 1;

if (…) {
       …
} else {
        switch (code.sym) {
        case SDLK_TAB:
                if (code.mod & KMOD_CTRL) {
                        if (code.mod & KMOD_SHIFT) {
                                if (selected_idx > max) {
                                        selected_idx = max;
                                } else if (selected_idx > 0) {
                                        --selected_idx;
                                }
                        } else {
                                if (selected_idx < max) {
                                        ++selected_idx;
                                } else if (selected_idx > max) {
                                        selected_idx = 0;
                                }
                         }
                …

From the forward (no SHIFT else) branch it looks like the intention is to make it go around, but (selected_idx > max) is not possible, so it doesn't happen. If so, I think what was meant is:

if (++selected_idx > max) {
        selected_idx = 0;
}

and so the backward (with SHIFT) branch should be:

if (--selected_idx > max) {  // now I get it, it's because of uint32_t underflow
        selected_idx = max;
}

Is that so?

If yes, I'll correct it with my mousewheel changes, because I intend to add handling of CTRL+[LEFT/RIGHT] too. At least I added handling of movement keys to other UI elements as well, because I think it's similar functionality to the wheel.

Also, if going around is intended for TAB, then I'll make it so for the wheel as well.

Top

Quote

If wrap around with TAB is not intended though, then shouldn't selected_idx > max revert it to selected_idx = max in both cases?

Top

Quote

But doesn't this mean wraparound? If it's not > 0, ie. if it is 0 (it's unsigned after all), then it becomes max, while max or greater (should not be possible) becomes 0. So if it's just a safeguard, and we don't want wraparound, then shouldn't it be: if (idx > 0) {--idx;} else {idx = 0;} } else { if (idx < max) {++idx;} else {idx = max;}?

Anyway, another question:

I saw in the file's history that originally it worked with LEFT/RIGHT, which was changed to CTRL+[SHIFT+]TAB. May I add CTRL+{LEFT/RIGHT} too? With CTRL it wouldn't steal the arrows from the more general uses.

Top

Quote

OK, I'll change the tab code to the cleaned up and corrected version.

As for arrow map movement (should this be moved to a new topic?): Does anyone actually use fast/slow with them? With the current values, I have to watch really closely to even see the difference. That's why I thought using ctrl-left/right for tab selection would be more useful to me. Also spinbox uses the arrows (even with ctrl for fast change), so I thought it would be good for consistency to use them in other widgets too. But OK, I'll keep key handling changes to a minimum in the mousewheel changeset then.

Still, for map movement, may I change ctrl-arrows to move it almost by a screenful to make it more useful, and shift-arrows to a much smaller value? E.g. one or even just a half node distance? Though that would change with zoom level — while current values change with window size: I'm not sure which one is better. Anyway, in practice it doesn't make all that big a difference in the end… (((or should step sizes be configurable?)))

Top

Quote