Ticket #3715 (closed defect: fixed)

Opened 7 years ago

Last modified 7 years ago

Midnight Commander 4.8.17/4.8.18 - strange behaviour on sorting file-/folder lists (race condition?)

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

Description

Since mc 4.8.17 using the mouse to sort files via double clicking on the fields "n", "name", "size" "modify time" at the top of the panels, behaves rather strangely.

Now I know that the term "double clicking" might sound a bit strange, actually I quite often want to see files/folders sorted by modification date in descending order (most recent ones at the top), so it takes clicking twice to achieve that.

If you double-click rather quickly (or better, if you don't wait between the clicks for some time), the file/folder list gets sorted and directly after that mc "jumps" into the sub folder which was marked at that time or one step up in the tree (just do this quick sorting by double clicking on "modify time" twice and you will see what I mean).

If you wait about 1-2 seconds between those clicks for sorting the view, the normal behaviour can be observed.

I checked versions 4.8.16 - 4.8.18 and 4.8.16 is the last version not to be affected.

I know this might be a little hard to understand (it is quite difficult to explain also, so please bear with me), so I will give you an example:

1) mc shows the root of the directory tree on a linux system, so I see the sub folders /bin, /boot, /dev, /etc and so on

2) let's say the folder "/home" is in focus (so the coloured bar of mc showing which file/folder could be marked with "insert" is over "/home")

3) now quickly (= normal speed you would use to do a double click anywhere else) double click on "modify time" or any other sorting criteria offered at the top of of the panel to sort according to that criteria.

4) you will see mc do the sorting twice (first ascending then descending) and immediately after that it will "jump" to the /home directory

5) repeat the same double click again in /home where you just landed, and after sorting the sub folders in /home mc will again jump immediately but this time you will go one up in hierarchy and you will be back in the root of the directory tree.

6) repeat the same "maneuver" again but now wait 1-2 seconds between clicking on "modify time" and mc behaves well in that way (most of the time at least).

One interesting note (and a second hint to why I think there might be a race condition here) if the object in focus is a file and not a folder this strange behaviour does not seem to appear.

What do I mean by this?

7) let's say again we are in the root of the directory tree and there is a file under focus (just create one by "touch /test_file" and select that file via cursor or with your mouse)

8) now you can click on all sorting criteria as fast as you can, mc will behave as expected (as there is no folder in focus it could otherwise change into?)

I hope my explanation is somehow understandable.

Greetings,

AK

Attachments

mc-3715-sort-order-double-click.patch (453 bytes) - added by egmont 7 years ago.
Fix

Change History

comment:1 Changed 7 years ago by zaytsev

Wow, I can indeed reproduce this on 4.8.18, no idea what's going on though :-/ Maybe bisection would yield something? Must be somehow related to all the changes in the widgets system.

comment:2 follow-up: ↓ 4 Changed 7 years ago by egmont

I can also reproduce it. Really weird.

Note that the mouse protocols in terminal emulators don't have any concept of double clicking. So mc just simply receives two click+release events shortly after each other.

comment:3 Changed 7 years ago by egmont

  • Cc egmont added

comment:4 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 7 years ago by ak

Replying to zaytsev:

Wow, I can indeed reproduce this on 4.8.18,

Replying to egmont:

I can also reproduce it. Really weird.

Dont't get me wrong, but this is some kind of relief here as I really did not know how to describe that problem properly and if someone can really reproduce this very strange issue.

Just to make things a little more clear.

  • The first release I found to be affected was 4.8.17, maybe that helps to narrow it down slightly
  • I am running openSUSE (42.1, but 13.1 is also affected, didn't test with 13.2 or Tumbleweed), but I tested with packages from other distros, too (debian, fedora), so I was quite (but not 100%) sure it is not related to any distro-specific patches

It really looks as mc treats this quick "double clicking" as it was clicking three times, the first "jump" into the sub folder goes into the folder which has been in focus, while the second "jump" one up in the hierarchy seems to be due to the fact that the focus is on the ".." (= one step up in hierarchy) at the top of the file list after changing to the sub folder.

You can verify that in another way by choosing another sub folder after the first jump, then quickly double click a selection criteria and you will land in the pre selected sub folder instead of going one up.

And it gets even better, another observation supporting this is the following one (I just discovered).

Instead of putting the focus on a folder and do the "double click", you put the focus on a file associated with some extra action (like a tar.gz or a video/audio).

Now, instead of "jumping" mc will execute the action linked to that file (i.e. open it with the utar:// vfs or even play that audio/video).

In my first post (under point 7) I created an empty file and there is no action associated with that, so that was the (only?) reason I did not observe that behaviour then.

Just give it a try.

AK

comment:5 in reply to: ↑ 4 Changed 7 years ago by egmont

  • Priority changed from major to critical

Replying to ak:

It really looks as mc treats this quick "double clicking" as it was clicking three times

Not quite; if it was triple clicking at that position, it would be the same as a single click. It's more of double clicking followed by an Enter, or a third click at a different position.

And indeed: if the selected file is an executable, double clicking on the sort order does execute that file! Ouch!!!

At this point I guess we really need to dive into the code rather than guessing :)

I hope nobody minds that I'm increasing the priority, unwanted executing of a binary could be really dangerous.

comment:6 Changed 7 years ago by egmont

git bisect says:
54456a678f69cce3f988358bd0ed6b0d5ce98964 is the first bad commit

Changed 7 years ago by egmont

Fix

comment:7 Changed 7 years ago by egmont

Fix attached.

A condition pretty obviously got lost when porting the code.

Also note that double clicking on the line above "Name Size ..." also caused to enter the given directory or execute the file, due to this same mistake.

comment:8 follow-ups: ↓ 9 ↓ 10 Changed 7 years ago by egmont

Nitpicking:

IMO it would be nicer (stick to 0-based coordinates and emphasize that there are 2 lines at the top of the panel which are not filenames, also more consistent with the MSG_MOUSE_DRAG branch) to write:

  y = event->y - 2;  // instead of -1
  ...
  if (y >= 0 && y < lines)  // instead of > and <=

comment:9 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 7 years ago by mooffie

Replying to egmont:

Nitpicking:

IMO it would be nicer (stick to 0-based coordinates and emphasize that there are 2 lines at the top of the panel which are not filenames, also more consistent with the MSG_MOUSE_DRAG branch) to write:

  y = event->y - 2;  // instead of -1
  ...
  if (y >= 0 && y < lines)  // instead of > and <=


Great minds think alike. That's the version I was preparing before clicking "refresh" and noting that you've already fixed this ;-)

BTW, doesn't it bother you folks that double clicking in an empty space (beyond the last file) executes the last file/directory? This behavior exists in the old version as well. (This can be fixed by having a function that calculates the file under coordinates (x,y). That is, by factoring out the code in MSG_MOUSE_DRAG.)

Last edited 7 years ago by mooffie (previous) (diff)

comment:10 in reply to: ↑ 8 ; follow-up: ↓ 14 Changed 7 years ago by andrew_b

  • Status changed from new to accepted
  • Votes for changeset set to andrew_b
  • Version changed from master to 4.8.17
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19
  • Owner set to andrew_b

Replying to egmont:

Nitpicking:

IMO it would be nicer (stick to 0-based coordinates and emphasize that there are 2 lines at the top of the panel which are not filenames, also more consistent with the MSG_MOUSE_DRAG branch) to write:

  y = event->y - 2;  // instead of -1
  ...
  if (y >= 0 && y < lines)  // instead of > and <=

This and mc-3715-sort-order-double-click.patch are in the

branch: 3715_panel_sort_double_click
changeset:16b0726e0797002d5fc2d5479d67ffe02074d064

Thanks!

comment:11 in reply to: ↑ 9 ; follow-up: ↓ 12 Changed 7 years ago by egmont

Replying to mooffie:

Great minds think alike. [...] BTW, doesn't it bother you folks that double clicking in an empty space (beyond the last file) executes the last file/directory?

You won't believe me... you seriously won't :)

As I woke up this morning, my very first thought was that that "lines" referred to the height of the widget, and as such, if there's an empty space, I'm afraid clicking there will execute the last entry. And if that's the case, it's really bad.

And indeed that's the case.

So yup I fully agree with you, this should be changed as well.

comment:12 in reply to: ↑ 11 Changed 7 years ago by andrew_b

Replying to egmont:

And indeed that's the case.

So yup I fully agree with you, this should be changed as well.

And this isn't matched with this ticket topic. Let's create another ticket.

comment:13 Changed 7 years ago by egmont

Filed #3722.

comment:14 in reply to: ↑ 10 Changed 7 years ago by ak

Replying to andrew_b:

This and mc-3715-sort-order-double-click.patch are in the

branch: 3715_panel_sort_double_click
changeset:16b0726e0797002d5fc2d5479d67ffe02074d064

Just want to confirm that applying that patch to 4.8.18 fixed the problem for me.

Thanks for your quick responses and for fixing the issue.

AK

comment:15 Changed 7 years ago by andrew_b

  • Branch state changed from on review to approved

comment:16 Changed 7 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

comment:17 Changed 7 years ago by andrew_b

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