Ticket #2163 (closed defect: fixed)

Opened 14 years ago

Last modified 11 years ago

Rotating dash not removed

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

Description

When reading a large directory, mc shows a rotating dash in the upper right corner (if enabled in the config). This symbol is not removed when mc finishes reading the directory. (It disappears on a subsequent Ctrl-R or window resize, though).

The easiest way to test it: stand on a directory, press and hold the Enter key for a long time (mc toggles back and forth between two directories).

When mc finishes reading a directory, it should remove the rotating dash.

Attachments

mc-4.8.8-rotating-dash.patch (6.7 KB) - added by egmont 11 years ago.
remove rotating dash when finished rotating

Change History

comment:1 Changed 13 years ago by andrew_b

  • Priority changed from minor to trivial
  • Branch state set to no branch
  • Milestone changed from 4.7 to Future Releases

Changed 11 years ago by egmont

remove rotating dash when finished rotating

comment:2 follow-up: ↓ 3 Changed 11 years ago by egmont

This has been bugging me forever, and I don't know why I didn't take time to fix it much earlier. My system is fast enough that I rarely see the dash actually rotating, but I'm always disturbed by the artifact it leaves behind, it just looks ugly to me.

Could you please review the patch?

Whenever rotating the dash finishes, I redraw the character that should be there. I believe there are 2 cases only: if the menubar is shown (then it's a space char, using menubar color), or if it's hidden (then it's the upper right corner of the panel, using the main color).

[As a really minor bug, if the menubar is shown but the terminal is way too narrow to show it fully (e.g. less than 42 chars wide in English), I print a space although a letter (the last visible one from the cropped menubar line) should be printed. Honestly I don't care about it as mc is useless anyways on such narrow terminals :) ]

As a side note, I inverted the boolean arg of find_rotate_dash() to be consistent with the new rotate_dash() behavior and to be intuitive. For me, FALSE meaning to show something and TRUE meaning to hide it was counter-intuitive. I couldn't figure out where this rotating dash appears, though.

comment:3 in reply to: ↑ 2 Changed 11 years ago by andrew_b

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

Replying to egmont:

Could you please review the patch?

In general, patch is OK. Thanks!

Whenever rotating the dash finishes, I redraw the character that should be there. I believe there are 2 cases only: if the menubar is shown (then it's a space char, using menubar color), or if it's hidden (then it's the upper right corner of the panel, using the main color).

[As a really minor bug, if the menubar is shown but the terminal is way too narrow to show it fully (e.g. less than 42 chars wide in English), I print a space although a letter (the last visible one from the cropped menubar line) should be printed. Honestly I don't care about it as mc is useless anyways on such narrow terminals :) ]

I moved dash to the corner of the right/top panel from the corner of the screen. Menubar is untouched now.

As a side note, I inverted the boolean arg of find_rotate_dash() to be consistent with the new rotate_dash() behavior and to be intuitive. For me, FALSE meaning to show something and TRUE meaning to hide it was counter-intuitive. I couldn't figure out where this rotating dash appears, though.

This part of your patch I applied with small modifications as a separate commit.

Branch: 2163_rotating_dash.
Initial changeset:941f9a279a596bd9c11ebc73aa6e780999fb6c3a.

comment:4 follow-up: ↓ 5 Changed 11 years ago by egmont

Thanks! One thing, though, please do

- pos = (pos + 1) % sizeof (rotating_dash); 
+ pos = (pos + 1) % (sizeof (rotating_dash) - 1); 

sizeof() counts the terminating zero, hence a 5th phase, a caret also shows up.

I'm not sure about the widget infrastructure of mc, but are you sure that using midnight_dlg does not interfere with any actual dialog that might be open while the dash is rotated? (If you're sure then that's okay! :))

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

Replying to egmont:

sizeof() counts the terminating zero

Indeed.
In case of

const char rotating_dash[] = "|/-\\";

sizeof (rotating_dash) == 5.

In case of

const char *rotating_dash = "|/-\\";

sizeof (rotating_dash) == 4.

I changed declaration of rotating_dash.

I'm not sure about the widget infrastructure of mc, but are you sure that using midnight_dlg does not interfere with any actual dialog that might be open while the dash is rotated? (If you're sure then that's okay! :))

I hope everything is OK here. :)

comment:6 Changed 11 years ago by egmont

I believe in case of

char *rotating_dash = "|/-\\";

sizeof(rotating_dash) accidentally happens to be 4 because the size of a pointer is 4 bytes, and not because the length of the string contained there is 4.

comment:7 follow-up: ↓ 8 Changed 11 years ago by egmont

Yup, sizeof(rotating_dash) == 8 on x86-84.

Please stick to the [] declaration (that's when the length can be known in compile time) and the -1 offset. Or the hardwired 4 :) Or you can do

char rotating_dash[4] = "|/-\\"

in which case the trailing zero is not stored in memory at all and then sizeof gives 4, but you still have to manually maintain the length in that declaration line.

In case of "char *" the length might change at runtime hence sizeof() (which is evaluated at compile time) has no chance of knowing it, and it would be weird if the "const" modifier changed the behavior of sizeof().

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

Replying to egmont:

Please stick to the [] declaration (that's when the length can be known in compile time) and the -1 offset. Or the hardwired 4 :) Or you can do

char rotating_dash[4] = "|/-\\"

in which case the trailing zero is not stored in memory at all and then sizeof gives 4, but you still have to manually maintain the length in that declaration line.

Ok. Thanks.

comment:9 Changed 11 years ago by egmont

It's cool now, thx!

comment:10 Changed 11 years ago by zaytsev

You guys are doing awesome work! Just so that you know...

comment:11 Changed 11 years ago by mike.dld

It is still possible to get sizeof() == 4, not hard-code any magic numbers and make the variable immutable at the same time. The code would look like this:

const char rotating_dash[] = { '|', '/', '-', '\\' };

comment:12 Changed 11 years ago by egmont

Good point, Mike!

comment:13 Changed 11 years ago by angel_il

  • Votes for changeset set to angel_il

comment:14 Changed 11 years ago by slavazanko

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

comment:15 Changed 11 years ago by andrew_b

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

Merged to master: [d38589cbcaa9f4e72dd2cadc4a96d29deab9e94b].

git log --pretty=oneline 368a303..d38589c

comment:16 Changed 11 years ago by andrew_b

  • Status changed from testing to closed

comment:17 Changed 11 years ago by egmont

  • Status changed from closed to reopened
  • Resolution fixed deleted

Unfortunately the fix is not complete yet. The rotating dash is now drawn by using a widget which is not resized when mc is resized. So if you make mc's terminal wider, the rotating dash and the box corner when it's removed is drawn at an incorrect position.

comment:18 Changed 11 years ago by andrew_b

Strange.
It should be fixed in [a365be63a29b5f78766c0aef7e0b9fe80c258d82].

comment:19 Changed 11 years ago by egmont

Oops, sorry. My bad. It is indeed fixed.

(I forgot that I had downgraded to 4.8.9 because #3047 was driving me crazy.)

comment:20 Changed 11 years ago by andrew_b

  • Status changed from reopened to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.