Ticket #2684 (closed defect: fixed)

Opened 5 years ago

Last modified 4 years ago

Display corruption in panels after window shrink

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

Description

Scenario 1:

  • Go to a directory with more than a screenful of files.
  • With the down arrow, place the cursor on the file that is currently visible at the bottom of the panel. (Example: if you have files named "a", "b", ... "z", and your terminal is 80x24, then you may for example see the entries from ".." (up-dir) to "n". Place the cursor over file "n" in this case.)
  • Resize the window to be 1 line shorter.
  • Expected behavior: the file listing should be scrolled so that the currently selected file ("n" in our example) is visible (e.g. show files from "a" to "n").
  • Actual behavior: entries from ".." to "m" are shown, the cursor line disappears.

Scenario 2:

  • Place the cursor on the file that's the second from the bottom (e.g. if you see entries from ".." to "n", place the cursor on "m").
  • Resize the window in a single step to be 2 lines shorter. (If your window manager doesn't support this operation, then as a workaround, on top of ticket #2678's changes, you can enter the builtin viewer, resize there and then quit the viewer.)
  • Expected behavior: as above
  • Actual behavior: entries from "a" to "m" are shown and "m" is highlighted, so far this is okay, but then "m" is printed again where the separator line between the file list and the mini status should be.

Scenario 3, 4, etc.: Place the cursor on the 3rd (4th, etc.) visible file from the bottom, and shrink by 3 (4, etc. respectively) lines in one step. Actual behavior: as in scenario 2.

Attachments

mc-4.8.0-cursor-after-resize.patch (424 bytes) - added by egmont 5 years ago.
mc-4.8.1-cursor-after-resize-try2.patch (3.7 KB) - added by egmont 4 years ago.
A much better version
mc-4.8.1-cursor-after-resize-try3.patch (3.6 KB) - added by egmont 4 years ago.
Alternate version, equally good as try2

Change History

comment:1 Changed 5 years ago by egmont

The function select_item() is the one that fixes the layout to make sure that the selected file is scrolled to be within the viewport. However, this function is not called on resize.

Please see the attached patch. (Of course, there are multiple possible places to place this call, you might prefer a different location.)

Changed 5 years ago by egmont

comment:2 Changed 5 years ago by egmont

Side note: Similar bug happens when the window size is not changed, but you change an mc option that alters the panel size, e.g. you enable the hint bar. My patch happens to fix this particular case too, but I'm not sure it fixes all the cases. Probably it's not that important though :)

Changed 4 years ago by egmont

A much better version

comment:3 Changed 4 years ago by egmont

I attach a much better version.

The problem was not that the infrastructure wasn't there, it was mainly some code duplication between adjust_top_file (buggy implementation) and select_item (correct implementation).

The bugs in the probably legacy adjust_top_file, which was executed at resize, are:

  • off by one error: if the selected file is just below the visible area (panel->selected - old_top == llines (panel)), it does not adjust though it should.
  • ugliness: modifies top_file by a lot when a small adjustment would also do, causing large unnecessary jumps of the viewport on the screen.
  • ugliness: might leave some space unusused at the bottom, while files are scrolled out at the top.
  • no support for "brief" file listing.

I've slightly refactored the code by moving the correct implementation to adjust_top_file. I've also refactored this good implementation a little bit and added comment to hopefully make it clearer what's going on.

Please review and apply if looks good, thanks! I hope it can make it into the next stable (4.8.1.1); after all, a display corruption on the main screen is quite ugly.

comment:4 Changed 4 years ago by angel_il

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

branch: 2684_always_show_selected_file
parent: master

Please review

comment:5 Changed 4 years ago by egmont

Purely as a coding excercise for fun, I created an alternate version "try3". It is equally good as "try2". Pick whichever you personally prefer :)

I made some manual optimization in adjust_top_file for the code to be shorter, in the expense of probably being a tiny little bit harder to understand (but I explained it in the comment), more like in traditional "hackish" style.

The story is: in the big branch of adjust_top_file (in try2), if you reshuffle the four checks to a particular order, then there's no need for the other branch as now the big branch happens to return the desired value in that case too.

Changed 4 years ago by egmont

Alternate version, equally good as try2

comment:6 Changed 4 years ago by angel_il

ARGH! :)

comment:7 Changed 4 years ago by egmont

:D

(Feel free to ignore try3 :))

comment:8 Changed 4 years ago by andrew_b

  • Keywords stable-candidate added
  • Votes for changeset set to andrew_b

comment:9 Changed 4 years ago by angel_il

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

comment:10 Changed 4 years ago by andrew_b

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

comment:11 Changed 4 years ago by egmont

Can I has backport to 4.8.1.x pretty please? :)

Soon that's going to be the stable branch for a year or even more, it'd be nice not to leave it with display corruption on the main screen.

Thanks a lot!
e.

comment:12 Changed 4 years ago by slavazanko

  • Status changed from testing to closed
  • Keywords stable-candidate removed
  • Votes for changeset changed from committed-master to committed-master committed-stable
Note: See TracTickets for help on using tickets.