Ticket #2163 (closed defect: fixed)
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
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
- Attachment mc-4.8.8-rotating-dash.patch added
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: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: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: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