Latest Posts

Topic: tabpanel.cc use of TAB key

tothxa
Avatar
Joined: 2021-03-24, 12:44
Posts: 99
Ranking
Likes to be here
Posted at: 2021-08-11, 00:37

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
Nordfriese
Avatar
Joined: 2017-01-17, 18:07
Posts: 1456
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2021-08-11, 10:40

IIRC it's not intended to wrap around. The checks for <0 and >max are just sanity checks to ensure that weird selection indices are brought back within reasonable bounds (though I'm not sure if they can ever be triggered at the moment).

Personally I prefer not to have wraparound with the wheel because the current implementation allows a convenient way to move to the first or last tab with a mouse gesture, and it's consistent with other (non-Widelands) UI elements that support mousewheel tabbing (e.g. Ubuntu MATE window switcher and desktop switcher). But when using the keyboard we have Home and End to do that, so here wraparound might be nice to have though.


Top Quote
tothxa
Avatar
Joined: 2021-03-24, 12:44
Posts: 99
Ranking
Likes to be here
Posted at: 2021-08-11, 11:22

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
Nordfriese
Avatar
Joined: 2017-01-17, 18:07
Posts: 1456
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2021-08-11, 11:46

Right I guess it should be something like:

                         if (code.mod & KMOD_SHIFT) {
                                 if (selected_idx > 0) {
                                         --selected_idx;
                                 } else {
                                         selected_idx = max;
                                 }
                         } else {
                                 if (selected_idx < max) {
                                         ++selected_idx;
                                 } else {
                                         selected_idx = 0;
                                 }
                          }

Top Quote
tothxa
Avatar
Joined: 2021-03-24, 12:44
Posts: 99
Ranking
Likes to be here
Posted at: 2021-08-11, 21:29

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
Nordfriese
Avatar
Joined: 2017-01-17, 18:07
Posts: 1456
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2021-08-11, 21:39

Yes, I got those mixed up face-smile.png


The problem with Left/Right was that building windows consumed the map movement keys. Ctrl+Arrows and Shift+Arrows are also used for map movement (fast/slow) so this would regress this issue.


Top Quote
tothxa
Avatar
Joined: 2021-03-24, 12:44
Posts: 99
Ranking
Likes to be here
Posted at: 2021-08-11, 23:20

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
Nordfriese
Avatar
Joined: 2017-01-17, 18:07
Posts: 1456
Ranking
One Elder of Players
Location: 0x55555d3a34c0
Posted at: 2021-08-12, 10:30

arrow map movement: Yes, I use those, but +1 for making the difference larger and +1 for letting it depend on zoom level instead of window size. -1 for configurable step sizes, such detailed controls just add clutter for hardly any benefit imho.


Top Quote