Ticket #3250 (closed defect: fixed)

Opened 3 years ago

Last modified 2 years ago

mcview: multiple bugs, rewriting desired

Reported by: egmont Owned by: andrew_b
Priority: major Milestone: 4.8.14
Component: mcview Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

mcview contains multiple issues that are not trivial to address:

  • ticket:2283: scrolling often introduces a partial line on the top;
  • ticket:2292: CJK not wrapped correctly;
  • ticket:2291: inconsistent symbol for half CJK;
  • ticket:3248: allows horizontal scrolling in wrap mode;
  • ticket:2283#comment:7: the help viewer scrolls by a complete paragraph rather than a visual line;
  • wrapping and scrolling is quite complicated, yet mcview and the help system have their own buggy implementation, they should be built on the same codebase;
  • mcview's plain.c is pretty much a subset of nroff.c, resulting in significant code duplication, they could be merged to one;
  • the help viewer only wraps at spaces to avoid splitting words, this nice functionality is missing from mcview.

I find it quite hopeless to address these bugs one by one on top of each other. Instead, a clean rewrite of the viewer could easily address all these.

(Only the part that wraps and renders the file contents on the screen and handles scrolling up/down needs to be rewritten. The rest (UI buttons, ruler, etc., as well as the hex viewer) would stay the same. It's a relatively small piece of code to touch.)

I'm planning to do this.

Attachments

mc-3250-viewer-rewrite-v1.patch (64.1 KB) - added by egmont 3 years ago.
Reimplementation, v1
mc-3250-viewer-rewrite-v1a.patch (68.0 KB) - added by egmont 3 years ago.
Reimplementation, v1a (only minor stylistic updates)
mc-3250-viewer-rewrite-v2.patch (71.1 KB) - added by egmont 3 years ago.
Reimplementation, v2
mc-3250-viewer-rewrite-v3.patch (71.0 KB) - added by egmont 3 years ago.
Reimplementation, v3
0001-3282.patch (666 bytes) - added by egmont 3 years ago.
Fix for bug 3282 (colored tilde after eof)
0002-3250c12.patch (8.9 KB) - added by egmont 3 years ago.
Fix for comment 12 (jumping to EOL in first line)

Change History

comment:1 Changed 3 years ago by egmont

More bugs:

  • ticket:2132: does't make horizontal scroll to the found text
  • combining accents not supported (although they appear correctly e.g. in file listing)
  • ticket:3256: down/pagedown/end weirdnesses
  • there's an additional empty line before starting to show the mcview_eof (~) character
  • the mcview_eof character is not shown in format (nroff) mode
  • ticket:3282: mcview_eof is highlighted if search string is found at end of file
Last edited 3 years ago by egmont (previous) (diff)

comment:2 Changed 3 years ago by egmont

Combining accents: They are printed correctly (both with slang and ncurses) if they are part of a single tty_print_string() or similar call together with their base character. If separated from their base character to another invocation of tty_print_string(), or being printed with tty_print_anychar, they appear at an undesired position and likely get overwritten by the next character.

comment:3 Changed 3 years ago by egmont

You can track progress at https://github.com/egmontkob/mc-3250. I'll get back here when it's (almost) ready.

Changed 3 years ago by egmont

Reimplementation, v1

comment:4 Changed 3 years ago by egmont

Please test and review the first patch. I've reimplemented the "ascii" (a.k.a. plain text, that is, not hex mode) viewer, both its "wrap" and "unwrap" mode.

For testing the nroff rendering with actual manual pages, it's recommended to also apply the patches from #1539, #3243, #3244.

I have not yet ported the help viewer to this codebase (although I've added comments on how I'd do that), have not implemented wrapping at word boundaries, and have not implemented horizontal scrolling to the found text. Other than these, I believe that all the bugs mentioned above are fixed.

Letters of left-to-right languages, including (zero wide) combining accents, (1 cell wide) spacing marks that are not to be separated from their base letter (forming a 2 cell wide character), CJK characters are hopefully always displayed correctly. Nroff formatting works on all of these.

Scrolling by 1 line actually always scrolls by 1 line, no more partial lines on the top, no longer need to press the arrow twice to scroll by 1 line, etc.

A stress-test file is included in tests/src/viewer.

I tried to carefully document in the source what and why I'm doing. If anything's still unclear, let me know and I'll extend the comments.

At this point I believe that this viewer is in all aspects at least as good as the previous one, and is usually much better.

I'm waiting for a supportive feedback (and hopefully you accepting my patch and including in official mc) before moving forwards and addressing the remaining improvements.

Last edited 3 years ago by egmont (previous) (diff)

comment:5 Changed 3 years ago by andrew_b

It is better to make large changes in the viewer after next release.

There are 5 months since release 4.8.12. We're planning 4.8.13 in a few days.

comment:6 Changed 3 years ago by egmont

Perfectly agree.

There are a few minor cosmetic issues I'd still like to fix (no code change but clarify comments, etc.), and I'm pretty sure you'll have comments and questions too.

Changed 3 years ago by egmont

Reimplementation, v1a (only minor stylistic updates)

comment:7 Changed 3 years ago by andrew_b

Yes, it works! Cool! Thanks!

But I can't say that your patch is ready for use. I have some remarks:

  • first of all, I believe that best way to create the patch is git format-patch instead of git diff or diff -ruNp.
  • about your coding style which is differ than one used in mc:
    • K&R formatting style and tabs instead of spaces are trivially fixed with make indent command (GNU indent is used here);
    • please don't use the //-style comments;
    • please use explicit comparison of pointer with NULL and integer with 0;
  • you use hardcoded non-ASCII characters:
    +#define BASE_CHARACTER_FOR_LONELY_COMBINING 0x25CC  /* dotted circle */
    +#define PARTIAL_CJK_AT_LEFT_MARGIN  0x25C2  // ' '  /* '<' doesn't look that good */
    +#define PARTIAL_CJK_AT_RIGHT_MARGIN 0x25B8  // ' '  /* '>' doesn't look that good */
    

This is not good choice for non-Unicode systems. I believe that default values should be ASCII, and custom values should be get from skin. So we need to create required skin items at first and then use them here.

comment:8 Changed 3 years ago by egmont

Most of your comments are minor formatting issues – does it mean that you like the overall structure, the architecture of the code etc.? Of course I'll fix the formatting.

One small question: The old viewer was in plain.c, the new one is in ascii.c (mostly so that "diff" doesn't try to be "clever" while developing), but I can move the new code back to plain.c. Any preference here?

you use hardcoded non-ASCII characters:

As for BASE_CHARACTER_FOR_LONELY_COMBINING, the Unicode standard recommends that lonely combining marks are displayed on top of this character (this is a recommendation, not a strict rule, but I thought why not go for it). If the terminal is not UTF-8, this will be replaced by a '.' anyways later in the code. So this is the right thing to do here.

As for PARTIAL_CJK_AT_XX_MARGIN, again: if it's not printable in the terminal then it's later replaced by a dot. We could go for taking the character from the skin, but it sounds a bit of overengineering to me. I'm planning to go with the Space character in the final version. Any other symbol I believe is more distracting than useful. We can improve later if need arises. (These two symbols are only displayed in unwrap mode, since in wrap mode we never display a partial character. In unwrap mode, if we ever display an arrow, it'd be more useful to denote that there's any text to the left/right which you'll see if you scroll horizontally, rather than denoting a partial character.)

comment:9 Changed 3 years ago by egmont

Oops, there's a bug, most likely introduced by my patch.

Ubuntu Trusty (bash-4.3.11), mc-4.8.13 + my patch, terminal of 87 columns, F2 -> m (View manual page) -> bash

Press Down or Page Down multiple times. Scrolling in the manual suddenly stops in the middle of a sentence, the top bar says "24576/24576+", that is, rendering happens to end where the current growbuf ends. The scrolling code figures out we're already at EOF so there's nowhere to scroll any further, and doesn't try to read further data to growbuf.

Changed 3 years ago by egmont

Reimplementation, v2

comment:10 Changed 3 years ago by egmont

Please see v2. It fixes the growbuf bug and addresses the formatting issues (plus some more minor naming/doc cleanups).

Changed 3 years ago by egmont

Reimplementation, v3

comment:11 Changed 3 years ago by egmont

v3 is a minor bugfix (an optimization condition was incorrect) and minor cleanup.

There's one more potential bug: the calls to mcview_eol() stop at the current growbuf boundary, and don't read more data to find a newline. It needs further investigation if (and how) this can lead to temporarily incorrectly displaying the file, and of course should be fixed.

comment:12 Changed 3 years ago by egmont

Also, mcview_moveto_eol() [by default bound to Ctrl+E] is quite broken in current mcview, and is not fixed by my patch. Will address in the next version.

comment:13 Changed 3 years ago by egmont

A note on mcview_moveto_eol() [Ctrl+E]:

It's not clear to me what would be the most reasonable behavior.

(1) The current aim (even though buggy) is to jump to the right according to the first displayed row's width. This doesn't play nice with my intended solution to #3256 because not every row can be displayed on the top, it's a bit illogical that you can jump to the end for most of the lines, except for the last (height-1) lines of the file.

(2) Another nice approach would be to jump according to the max width of the rows that are currently displayed.

Yet another approach could be to look at the (3) max width of all the lines of the file (problematic with large files or growbuf), or the (4) max width of all the lines up to the current viewport (still problematic with large files).

I think I'll go for (2), sounds to me the best compromise between usability and ease of implementation. (Btw this is a really insignificant issue.)

comment:14 follow-up: ↓ 15 Changed 3 years ago by ossi

the current behavior (sans bugs) seems most useful to me. that's relevant when you have lines of vastly different length (happens often enough in log files, for example).
of course the choice of the first line as the orientation point is arbitrary ... that's an inevitable effect of bringing line-dependent navigation features into something that doesn't have a line cursor to start with.

the "search next" feature has the same problem.

comment:15 in reply to: ↑ 14 Changed 3 years ago by egmont

Replying to ossi:

the current behavior (sans bugs) seems most useful to me. that's relevant when you have lines of vastly different length (happens often enough in log files, for example).

Do you actually use this feature, would it be important for you that it works this way? I would have assumed this was a very rarely used feature (I just discovered it yesterday by studying the code). I would hate to modify the vertical scrolling boundaries just because of this feature, I'd rather sacrifice this one a bit.

of course the choice of the first line as the orientation point is arbitrary ... that's an inevitable effect of bringing line-dependent navigation features into something that doesn't have a line cursor to start with.

Yup, indeed... the cause of so many problems :)

the "search next" feature has the same problem.

Haven't thought of "search next", will put some thinking into it. Firefox always continues from the last match, no matter where you scrolled in the mean time - something I'm not quite satisfied with. I believe continuing from the last match, unless you scrolled, in which case continue from the top sounds a good solution. This could work reasonably well with my preferred way of limiting vertical scrolling, so basically jumping to eol is the only problematic issue.

comment:16 follow-up: ↓ 17 Changed 3 years ago by ossi

i didn't know about this feature until reading this bug, either - it's awesomely well hidden (it doesn't appear in the UI, the online help, or the man page).
however, i *missed* it often enough, and now that i know about it, i'd be annoyed by it being removed/rendered useless.

regarding "search next", it's even more complicated. it's annoying that the hit appears in the first line - i often enough need to scroll back to see more context. which affects "search next" adversely, obviously.

i have no clue what to do about these issues, short of introducing a cursor (which would obviously have downsides of its own. but then, the gnu info browser also has a cursor).

comment:17 in reply to: ↑ 16 Changed 3 years ago by egmont

Replying to ossi:

i didn't know about this feature until reading this bug, either

Guess I shouldn't have mentioned it haha :D Seriously, I think either (1) or (2) above would be good enough, wouldn't it?

regarding "search next", it's even more complicated. it's annoying that the hit appears in the first line - i often enough need to scroll back to see more context. which affects "search next" adversely, obviously.

There's obviously room for improvement, like less's -j N option (to show N lines of context backwards), or to show all the matches at once, maybe even combined with less's -a/-A, etc...

In this ticket I'll make "search" not worse than in current mc, and Ctrl+E not significantly worse (although, as said, it'll have some new limitations). Later we can improve them.

i have no clue what to do about these issues, short of introducing a cursor (which would obviously have downsides of its own. but then, the gnu info browser also has a cursor).

I don't know either, but I think we should look at modern UIs as references (such as graphical viewers, browsers, mobile apps etc.) rather than other ancient terminal apps (like info).

comment:18 follow-up: ↓ 27 Changed 3 years ago by ossi

(2) would already sufficiently cripple the feature to be useless in some cases. imagine a bunch of log lines that are "normal" width, two relatively close to each other that are 1000 cols, and one between them that is 500 cols. with your approach, there is no reasonable way to navigate to the end of the latter one.

"let's do <...> first and improve then" isn't a very trustable proposition when it is already known that some things will be broken by the first step and one has no clue how to do the second one.

modern uis have a horizontal scroll bar. this is obviously not useful at all without resorting to a pointing device.

maybe this would work: introduce a cursor, but with swapped move vs scroll keys compared to the editor.

comment:19 follow-ups: ↓ 21 ↓ 22 Changed 3 years ago by angel_il

egmont: i'm really like old behavior, when i can vertical scroll over EOL.

also:
1) set mcview_eof=~
2) create file with

1
2
3
4
\n

3) open in old mcview
4) open in new mcview (with patch)

in old mcview i'm see

1
2
3
4

~
~
~

in new mcview

1
2
3
4
~
~
~
Last edited 3 years ago by andrew_b (previous) (diff)

comment:20 Changed 3 years ago by andrew_b

I've created the branch:
3250_mcviewer
Initial changeset:d3a4b26415f7e2560107b0b0d4e884fb9841f532

I think, the other patches can be created on top of HEAD of 3250_mcviewer.

comment:21 in reply to: ↑ 19 Changed 3 years ago by andrew_b

Replying to angel_il:

egmont: i'm really like old behavior, when i can vertical scroll over EOL.

Discuss that in #3256, please.

comment:22 in reply to: ↑ 19 ; follow-up: ↓ 23 Changed 3 years ago by egmont

Replying to angel_il:

also:
1) set mcview_eof=~
2) create file with

1
2
3
4
\n

[...]

Can't reproduce, probably I misunderstand you. What's the *exact* contents of the file? Could you please attach it, or inline the hexdump?

comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 3 years ago by andrew_b

Replying to egmont:

Can't reproduce, probably I misunderstand you.

He wants view the '\n' at the EOF as an empty line below file content.

comment:24 in reply to: ↑ 23 Changed 3 years ago by egmont

Replying to andrew_b:

Replying to egmont:

Can't reproduce, probably I misunderstand you.

He wants view the '\n' at the EOF as an empty line below file content.

I still don't know the *exact* contents of the file. I don't understand his way of describing it, e.g. 1 and 2 are above each other, so I assume there's a '\n' in between them, the same for 4 and '\n', so I guess the file ends in two '\n's, but why write some of them implicitly and the last one explicitly? I don't understand the language he uses to describe the file contents. Please show me the *very exact* file, attached to this ticket, or described by a hexdump or by a shell command that creates it, where old and new mcviewer differs in behavior and the new one is considered buggy.

In my understanding, his file is the sequence of these bytes: 31 0a 32 0a 33 0a 34 0a 0a, in which case I get the expected behavior, not the one he described for new mcviewer.

Show me the *exact* file, please.

comment:25 Changed 3 years ago by egmont

I can reproduce this with the file that ends in one single newline character, that is, its hexdump is 31 0a 32 0a 33 0a 34 0a.

This is one more place where I firmly believe old mc is buggy and the new one is correct.

The new behavior is the one that matches vim, less, and even a simple cat.

The new behavior matches the meaning of '\n', which is a line terminator, rather than separator. The last line of this file is "4", and not an empty line after the last '\n'.

Changed 3 years ago by egmont

Fix for bug 3282 (colored tilde after eof)

Changed 3 years ago by egmont

Fix for comment 12 (jumping to EOL in first line)

comment:26 Changed 3 years ago by egmont

Please apply these two patches I've just uploaded. One of them fixes #3282, the other one fixes comment 12 (makes Ctrl+E jump exactly to the end of the first line, even if that line contains all kinds of weird stuff like CJK, combining, tabs, nroff, whatever).

comment:27 in reply to: ↑ 18 Changed 3 years ago by egmont

Replying to ossi:

(2) would already sufficiently cripple the feature to be useless in some cases.

Okay, I agree, indeed (1) [jumping to EOL on a particular line] is better than (2) [jumping to the max of EOLs in the visible lines].

"let's do <...> first and improve then" isn't a very trustable proposition when it is already known that some things will be broken by the first step and one has no clue how to do the second one.

I didn't say I had no clue at all :)

maybe this would work: introduce a cursor, but with swapped move vs scroll keys compared to the editor.

This would introduce a visual clutter for all the use cases, although in the vast majority of uses that cursor would be unused.

Jumping to EOL will probably be the least used feature of mcview, maybe you being the only person using it. I really wouldn't like to see any of the basic features used by pretty much all users suffer a degradation just because of this. SW development is usually about finding the right compromise (you hardly ever get all the features together perfectly), and this time jumping to EOL seems to be by far the least significant, the one that's the best to sacrifice if such a sacrifice has to be made. There are still plenty of workarounds, like Ctrl+Left / Ctrl+Right a couple of times (moving by 10 columns), or enabling wrap mode, or using mcedit...

That being said, my current recommendation (also hearing angel_il's opinion):

Make mcview default to what's currently in the branch plus my recent patches. This way scrolling would be limited to the point where the last line of the file reaches the bottom of the screen, and hence jump-to-eol would be available for all but the last height-1 lines.

And have a hidden config option for experts that let you enable scroll beyond eof, until the last line of the file reaches the top of the screen. (The exact details of how End and Page Down behave could exactly match current mc, or could be fine tuned if required.) If that option is enabled, the search result can always show up on the top line, the Goto Line feature always brings the desired line to the top, and Jump To Eol becomes available for all lines of the file.

I'm happy to implement this if this sounds like a good enough solution for everyone. (Note that mcview_eof=~ is already such a hidden option, and I guess there's a noticeable correlation between people who want to scroll beyond eof and people who want to see ~ chars.)

Last edited 3 years ago by egmont (previous) (diff)

comment:28 follow-up: ↓ 29 Changed 3 years ago by ossi

so what again was the reason for avoiding scrolling past eof? it doesn't seem that terrible to me at all (except when using newlines as separators (instead of terminators), which would mean that the last "visible" line is usually empty).

comment:29 in reply to: ↑ 28 Changed 3 years ago by egmont

Replying to ossi:

so what again was the reason for avoiding scrolling past eof?

See #3256 for my reasons. I was originally planning to discuss the desired behavior there. I didn't think that it would interfere with this ticket (making it a bit inconvenient to move the conversation there, which we should probably still do), and that my suggestion would break the probably least important feature of mcview that probably no-one cares about except for you ;)

comment:30 Changed 3 years ago by egmont

Another idea: We should make sure that the search string is scrolled in horizontally (#2132), but the file is scrolled to the leftmost possible column that fulfils this criterium. Then searching for the regexp $ should give the exact same behavior as Ctrl+E, with the addition that the next search starts from the previous one, so when the file is scrolled to the bottom it keeps going over the lines and scroll to the end of the next one (even though the "cursor" is invisible). This would again provide a(n almost?) perfect solution for jumping to eol in those lines.

comment:31 follow-up: ↓ 32 Changed 3 years ago by ossi

ok, let's assume ctrl-e continues to exists as a shortcut (and gets properly documented, otherwise it's just as non-obvious as searching for rx $). it should not actually do a regexp search (because that would alter the search history), but should just use the search navigation and highlight mechanism directly.

there should be a nice way to visualize newline hits. i actually have a patch for newline visualization in the editor. it may even be on the tracker, otherwise it's in the list archive. it's not rocket science, anyway.

now, the real challenge is implementing sane search navigation semantics. maybe this will work:

  • a new search starts at the top of the screen (and backwards search starts at the bottom)
  • searching continues from the current hit, regardless of where it is
  • scrolling does not affect the search cursor. even if new hit candidate comes into view. only when the cursor leaves the screen, we treat it as a new search.
  • the highlighting of the hit must persist as long as it is valid, i.e., until it leaves the view

on top of that, it should be possible to implement displaying context above/below the search hit. note that the search would still start at the first/last visible line and the screen would never be scrolled back/forward to create more context.

comment:32 in reply to: ↑ 31 Changed 3 years ago by egmont

Replying to ossi:

now, the real challenge is implementing sane search navigation semantics. maybe this will work:

I believe this should be discussed and implemented separately. I'm not happy with the current behavior either, but I don't want to solve every single issue in this ticket and don't want this one to be blocked.

Btw, the real challange is not *implementing* sane search navigation semantics, the real challenge is *coming up* with those semantics, implementing shouldn't be hard – I guess this is what you meant too.

comment:33 follow-up: ↓ 35 Changed 3 years ago by ossi

i already expressed my opinion about "let's break it first and possibly fix it later". ;)
while the ctrl-e feature is obscure enough to leave it aside for now, your plan will cause very visible regressions in how the search function works if you don't consider the above points. no plan, no go.

comment:34 follow-up: ↓ 36 Changed 3 years ago by ossi

and now for something completely different ...

a search hit display that fits a cursor-less scroll area much better than a hit cursor of any kind is the "highlight all visible hits simultaneously" type. search next/previous would just ensure that the next/previous hit comes into view (with sufficient context).
a hotkey to disable hit highlighting (like less' alt-u) would be added. searching for the empty string should have the same effect.

however, i can think of two problems with this approach:

  • it doesn't work with horizontal scrolling, because "next" becomes ambiguous
  • more or less for the same reason, it would not cover the ctrl-e case

that's too bad, because i actually like the idea.
i guess this would work:

  • the visualization part can be an always available option
  • the navigation part would be available (and always enabled when the visualization part is?) only in wrapped mode

comment:35 in reply to: ↑ 33 Changed 3 years ago by egmont

Replying to ossi:

i already expressed my opinion about "let's break it first and possibly fix it later". ;)

Interestingly, 4.8.13 was released with a severe known regression, against my explicit "no" vote, remember? Where were you then?

Here I'm breaking the probably by far least used and least important feature that probably noone but you uses. And I do have a proposal on how to fix that (the config option that lets you scroll beyond eof). I asked you if that solution would be good enough for you, you haven't responded yet.

I'm asking again: Would that config option be good enough for you?

while the ctrl-e feature is obscure enough to leave it aside for now, your plan will cause very visible regressions in how the search function works if you don't consider the above points. no plan, no go.

I don't get it. Could you please describe in details: What are the regressions caused in the search function by my current patches?

You tell me I have no plan? I do have a plan. Let's see how the nowadays by far most frequently used document viewers (that is: web browsers) handle these issues, and do what they do. That's my plan. Is it good enough for you?

I find it ridiculous that I come up with a complete reimplementation solving dozens of bugs that would have been truly painful to solve one by one, putting about a week's work, even offering to implement the previous scrolling behavior, and then suddenly here you are trying to block this whole thing because you can't imagine that searching could work on top of this. With the scroll-beyond-eof option that I offered to implement as a possible hidden config, you'll be back wrt. search exactly where current mcview is, except for all those bugs. And later we can improve search – it sure deserves improvement. But that'll be a separate step on top of this one, probably done by someone else. (You really know what it takes to make someone lose motivation.)

comment:36 in reply to: ↑ 34 Changed 3 years ago by egmont

Replying to ossi:

and now for something completely different ...

If it's something completely different, it should go to a separate ticket. I'm not going to fix in this ticket every single bug mcview contains and implement every single feature anyone has ever requested. In this ticket I'm rewriting mcview to be much better in most of the areas than the current one, paying the price of becoming a tiny bit worse in the least significant part. That's all there is to it. Any further improvements should be separate stories.

comment:37 Changed 3 years ago by egmont

At this point I find this conversation absolutely pointless, counterproductive, waste of my time. Ossi and I know but don't understand each other's opinion. I'm truly unhappy that someone tries to block my work based on made up scenarios – I haven't heard any concrete bug how my current version breaks search – or because of a minor regression in the least used feature. If there are any bugs in the current implementation, I'd be happy to hear what those are and fix them.

I'd like to hear the opinion of official mc developers who have a say on whether my patch will be accepted or not, and what I need to do to get it accepted.

comment:38 follow-ups: ↓ 39 ↓ 40 Changed 3 years ago by ossi

Replying to egmont:

Interestingly, 4.8.13 was released with a severe known regression, against my explicit "no" vote, remember? Where were you then?

i can't be everywhere. also, you know that i don't have any authority regarding the release process.

I'm asking again: Would that config option be good enough for you?

no, it wouldn't. micro-options are evil. and in this case, it would be simply a cop-out.

Could you please describe in details: What are the regressions caused in the search function by my current patches?

the search next function relies on the first line. it simply won't work any more when you can't scroll beyond eof and you don't implement an alternative cursor mechanism.

Let's see how the nowadays by far most frequently used document viewers (that is: web browsers) handle these issues, and do what they do. That's my plan. Is it good enough for you?

that's not a plan. it's an idea.
i offered you a concept.
you can make it a plan.

also, i actually tested firefox before i posted my comments, and i found it awful. it doesn't invalidate the hit position when it goes out of view (which it should, at least for vertical scrolling). and this actually *is* a problem - i regularly use it as a browser for the Qt CI's huge logs, and it bites me over and over again, because it's simply non-intuitive.

I find it ridiculous that I come up with a complete reimplementation solving dozens of bugs that would have been truly painful to solve one by one, putting about a week's work, even offering to implement the previous scrolling behavior, and then suddenly here you are trying to block this whole thing because you can't imagine that searching could work on top of this.

your re-implementation is most welcome. however, it does in no way imply that you must change the current scrolling behavior (or hide it behind an option). it's an orthogonal issue that should be considered separately.

Replying to egmont:

If it's something completely different, it should go to a separate ticket.

i suppose you missed the monty python reference, otherwise you'd notice that it was a joke.
it is very much pertinent to the bigger picture of changing the scrolling behavior and making the search function still work with it.
and yes, it's big enough to be a separate ticket (maybe it already exists?), but it would be a dependency of the scrolling behavior discussion anyway.

comment:39 in reply to: ↑ 38 Changed 3 years ago by egmont

Replying to ossi:

the search next function relies on the first line. it simply won't work any more when you can't scroll beyond eof and you don't implement an alternative cursor mechanism.

Have you *tried* the search next function with my version? It works for me. If you have any issues, please describe them.

comment:40 in reply to: ↑ 38 ; follow-up: ↓ 41 Changed 3 years ago by egmont

Replying to ossi:

your re-implementation is most welcome. however, it does in no way imply that you must change the current scrolling behavior (or hide it behind an option). it's an orthogonal issue that should be considered separately.

Strictly speaking, the issue is indeed orthogonal. However, I couldn't use the old code, I would have needed to reimplement, and I really didn't want to reimplement something that in my opinion is fundamentally broken by design and offers a broken user experience. There's where the correlation comes from.

I could easily reimplement any other previous bugs, like the one from comment 19, I just would prefer not to, but rather go for the better solution.

If really required, I'll reimplement the previous scrolling behavior. I'm waiting for the official developers' word on this. I just hoped that mc could go for the IMO saner solution in the same step.

comment:41 in reply to: ↑ 40 ; follow-up: ↓ 42 Changed 3 years ago by ossi

Replying to egmont:

Have you *tried* the search next function with my version?

no, i haven't. so far, you seemed hellbent on making a point that there isn't actually an issue in the first place, rather than pointing out that you actually addressed it. so, how did you address it? i don't want to test it to figure it out myself.

Replying to egmont:

Strictly speaking, the issue is indeed orthogonal.

stop right here.
have you ever heard about commit atomicity? one self-contained change at a time?

comment:42 in reply to: ↑ 41 ; follow-up: ↓ 43 Changed 3 years ago by egmont

Replying to ossi:

no, i haven't. so far, you seemed hellbent on making a point that there isn't actually an issue in the first place, rather than pointing out that you actually addressed it. so, how did you address it? i don't want to test it to figure it out myself.

Then what are you talking about???

"search next" finds from the last match, not from the topmost line (assuming you don't move with the cursor or whatever). I haven't touched this code, therefore it still works.

You're trying to convince me that my code isn't good because you imagined there might be a bug??? And you're refusing to try out the code to realize it's actually working???

Please show me existing bugs, not imaginary ones!

Replying to egmont:

Strictly speaking, the issue is indeed orthogonal.

stop right here.
have you ever heard about commit atomicity? one self-contained change at a time?

In this bug I've addressed N (about 10) issues because addressing them separately wouldn't have been feasiable. I wanted the scroll-beyond-eof bug to be one of them, so I asked if it was okay for everyone. Without that, I might address only N-1 issues and 1 remaining, with noticeable extra work.

You don't want me to split my change to N-1 consecutive ones, each addressing 1 single issue, do you?

Addressing that +1 issue in the same change would have saved noticeable work for me, if people here agreed that that's the right way to go.

Look at my other tickets/patches/contributions to other projects as well, you'll realize that I indeed know about commit atomicity. There are just places where it doesn't make sense to be pushed beyond all boundaries. Like when it would cause extra work due to deliberately reimplemeting a braindamaged behavior, and then removing that code in the next step.

comment:43 in reply to: ↑ 42 ; follow-up: ↓ 44 Changed 3 years ago by ossi

Replying to egmont:

"search next" finds from the last match, not from the topmost line (assuming you don't move with the cursor or whatever). I haven't touched this code, therefore it still works.

oh, good. i missed that, because a) it always jumps to the top and b) any scrolling resets the match, so the two are virtually indistinguishable ... unless there are multiple hits on one line. you could have simply pointed that out to show that my assumptions were wrong instead of going all defensive.

In this bug I've addressed N (about 10) issues because addressing them separately wouldn't have been feasiable.

so far that makes sense (at least in theory - i didn't bother checking whether it's true. from experience i can say that people make excessive use of that claim).

I wanted the scroll-beyond-eof bug to be one of them, so I asked if it was okay for everyone.

well, you got your answer.
and as you can see from the discussion, you totally shot yourself in the foot with this intentional non-atomicity.

Like when it would cause extra work due to deliberately reimplemeting a braindamaged behavior, and then removing that code in the next step.

that's actually a completely reasonable thing to do when the issues being addressed are orthogonal and the code overlap is minimal (we are talking about ~3 if()s here).

comment:44 in reply to: ↑ 43 Changed 3 years ago by egmont

Replying to ossi:

you could have simply pointed that out to show that my assumptions were wrong instead of going all defensive.

I would have done it, had I understood the situation. It would have never occurred to me that you're talking about hyphotetical nonexisting bugs rather than actual ones. In this regard, isn't talking about imaginary bugs without trying out the code (and at least stating it explicitly) offensive instead? Nevermind.

I wanted the scroll-beyond-eof bug to be one of them, so I asked if it was okay for everyone.

well, you got your answer.
and as you can see from the discussion, you totally shot yourself in the foot with this intentional non-atomicity.

Like I should've known in advance that some folks will insist on the ancient braindamaged behavior rather than the only one that is widely adopted in all kinds of sw products and matches common sense... In my opinion, being able to scroll beyond EOF is a *bug* and I just wanted to include that bugfix in the rewrite too. I didn't want to reimplement that bug. I'm sad to see that some folks insist on this crappy behavior for the sake of a way less important feature.

that's actually a completely reasonable thing to do when the issues being addressed are orthogonal and the code overlap is minimal (we are talking about ~3 if()s here).

... and my employer pays for my work even if takes longer time, instead of spending my free time... and if the roundtrip time of valuable feedback is not measured in weeks but in hours. Yes it's a reasonable thing to do more work for orthogonal changes in this case. My 2 cents, YMMV.

comment:45 Changed 3 years ago by egmont

To hopefully put aside this discussion for now:

In this ticket I will implement any EOF behavior that mainstream mc developers ask me to implement. This could be to leave my patch as-is, or to reimplement the old behavior, or a dual one based on a config option, or whatever.

But: I'd like to see an explicit request from the mainstream developers first. I won't proactively go in the direction that I myself find the wrong way and argue against.

Once this ticket is closed, we might continue the discussions and code changes in #3256.

comment:46 Changed 3 years ago by andrew_b

Branch 3250_mcviewer was rebased to recent master.
Initial changeset: [15e8b2eb4ad8e8ad11e2ec6735dd1126eccda188].

Currently, this branch contains 5 commits and I'd like to squash them into single one. Egmont, since you're the author of code, сould you write more or less detailed comment for commit, please? Thanks!

comment:47 Changed 3 years ago by egmont

How about something like this? I'm very bad at writing changelogs, feel free to edit as you please.

Tickets #3250, #3256: Rewrite mcview's rendering and scrolling

Major rewrite of mcview's parts responsible for rendering and scrolling the contents:
- No more partial lines at the top and failure to scroll when Up or Down is pressed
- Better handling of CJK characters
- Handle combining accents
- Improved nroff support
- More conventional scrolling behavior at the end of the file

comment:48 Changed 3 years ago by andrew_b

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

Branch 3250_mcviewer was rebased to recent master.
All commits are squashed: [90152a17c9c860d0d5b58daacb0ced6756506e7b].

comment:49 Changed 3 years ago by andrew_b

  • Votes for changeset set to andrew_b

comment:50 Changed 2 years ago by andrew_b

  • Branch state changed from on review to approved

comment:51 Changed 2 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:52 Changed 2 years ago by andrew_b

  • Status changed from testing to closed

comment:53 Changed 2 years ago by egmont

The help viewer was not addressed in this patch; I've opened a new ticket #3396 to track that.

Note: See TracTickets for help on using tickets.