Ticket #3554 (closed defect: fixed)

Opened 8 years ago

Last modified 8 years ago

Listbox shouldn't wraparound on mouse scroll

Reported by: egmont Owned by: andrew_b
Priority: minor Milestone: 4.8.16
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Open a listbox such as Alt-h. Use mouse or touchpad scroll. Notice it wraps around.

Wrapping around may be okay for keyboard up/down (although I'm not even sure about it, as let's say the main panels don't wrap).

For touchpad/mouse it's definitey undesired, since you'd expect a single fast/long scroll motion to end up at the first/last item, not go spinning around and around.

Attachments

0001-listbox-code-cleanup.patch (4.8 KB) - added by mooffie 8 years ago.

Change History

comment:1 Changed 8 years ago by ossi

wrapping is also inconsistent with other ui elements (panels don't wrap, and editors and viewers naturally don't, either.
wrapping can be an optimization in some cases, but it's unintuitive enough to be not useful imo (i always hit 'end' first anyway). i'm all for removing it.

comment:2 Changed 8 years ago by mooffie

Here's a minor code cleanup patch that makes it trivial to fix the wraparound issue.

But this patch would be nice to commit regardless of what we decide to do, if at all, with the wraparound issue. Maybe I should have posted it in a separate ticket?

The patch replaces every:

for (...)
    listbox_back/fwd (l);

with:

listbox_back/fwd_n (l, ...);

This makes the code easier to decipher (...and it leaves listbox_fwd/back() to the exclusive use of the up/down keyboard or mouse keys, which makes fixing the wraparound issue trivial).

Changed 8 years ago by mooffie

comment:3 follow-up: ↓ 4 Changed 8 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.16

Branch: 3554_listbox_wrap
Initial changeset:99dd99172f4c61ff36450a56cb08634bbd24bad4

Last edited 8 years ago by andrew_b (previous) (diff)

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 8 years ago by mooffie

Replying to andrew_b:

Initial changeset:99dd99172f4c61ff36450a56cb08634bbd24bad4


This works for me. Excellent.

BTW, the following...

if ((guint) l->pos + 1 < g_queue_get_length (l->list))

...can now be changed to:

if (l->pos < LISTBOX_LAST (l))

comment:5 in reply to: ↑ 4 Changed 8 years ago by andrew_b

Replying to mooffie:

...can now be changed to:

There is no need to call g_queue_is_empty() here.

comment:6 Changed 8 years ago by zaytsev

I guess he simply meant that it's a bit clearer :-)

comment:7 Changed 8 years ago by andrew_b

The new hidden option 'listbox_wraparound_mouse_scroll' was added to enable/disable listbox wraparound on mouse scroll. Enabled by default to keep current behaviour.

Does anybody vote for this ticket? The last comment was 4 weeks (a month) ago.

comment:8 follow-up: ↓ 9 Changed 8 years ago by ossi

and yet another useless micro option ...

at least switch the default to the new behavior. if somebody looks for the old one, they'll easily find where it has gone.

comment:9 in reply to: ↑ 8 Changed 8 years ago by andrew_b

Replying to ossi:

and yet another useless micro option ...

at least switch the default to the new behavior. if somebody looks for the old one, they'll easily find where it has gone.

There is no problem to remove last commit. I'm not insist on this option.

comment:10 Changed 8 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:11 Changed 8 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [1cf18abd62bcba6d636ee955eb29fc9b26548756].

git log --pretty=oneline a5cd009..1cf18ab

without micro option.

comment:12 Changed 8 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.