Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Display corruption in panels after window shrink #2684

Closed
mc-butler opened this issue Dec 4, 2011 · 15 comments
Closed

Display corruption in panels after window shrink #2684

mc-butler opened this issue Dec 4, 2011 · 15 comments
Labels
area: core Issues not related to a specific subsystem prio: low Minor problem or easily worked around ver: 4.8.0 Reproducible in version 4.8.0
Milestone
4.8.2

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/2684
Reporter egmont (@egmontkob)

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 Cmdline cursor misplaced after a resize in viewer #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.

Note

Original attachments:
@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Dec 5, 2011 at 16:54 UTC (comment 1)

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.)

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Dec 5, 2011 at 16:54 UTC

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Dec 5, 2011 at 20:55 UTC (comment 2)

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 :)

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Jan 18, 2012 at 12:18 UTC

A much better version

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Jan 18, 2012 at 12:26 UTC (comment 3)

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.

@mc-butler
Copy link
Author

Changed by angel_il (@ilia-maslakov) on Jan 19, 2012 at 7:00 UTC (comment 4)

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

branch: 2684_always_show_selected_file
parent: master

Please review

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Jan 19, 2012 at 9:05 UTC (comment 5)

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.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Jan 19, 2012 at 9:08 UTC

Alternate version, equally good as try2

@mc-butler
Copy link
Author

Changed by angel_il (@ilia-maslakov) on Jan 19, 2012 at 9:17 UTC (comment 6)

ARGH! :)

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Jan 19, 2012 at 9:19 UTC (comment 7)

:D

(Feel free to ignore try3 :))

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Jan 22, 2012 at 6:37 UTC (comment 8)

  • Votes set to andrew_b
  • Keywords set to stable-candidate

@mc-butler
Copy link
Author

Changed by angel_il (@ilia-maslakov) on Jan 25, 2012 at 11:31 UTC (comment 9)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 23, 2012 at 13:03 UTC (comment 10)

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

Merged to master.
[94ffb8b]

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 23, 2012 at 14:32 UTC (comment 11)

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.

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Mar 7, 2012 at 8:43 UTC (comment 12)

  • Votes changed from committed-master to committed-master committed-stable
  • Keywords stable-candidate deleted
  • Status changed from testing to closed

Applied to stable:
[5cebdc4]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: core Issues not related to a specific subsystem prio: low Minor problem or easily worked around ver: 4.8.0 Reproducible in version 4.8.0
Milestone
4.8.2
Development

No branches or pull requests
1 participant
@mc-butler