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

A help shows wrong #3754

Closed
mc-butler opened this issue Jan 9, 2017 · 18 comments
Closed

A help shows wrong #3754

mc-butler opened this issue Jan 9, 2017 · 18 comments
Assignees
@mooffie
Labels
area: docs Improvements or additions to documentation prio: low Minor problem or easily worked around
Milestone
4.8.19

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3754
Reporter rimf (rimf@….ru)
Mentions egmont (@egmontkob)
Keywords help, doc, tab

Built-in help system incorrectly handles the tab character. The format of the help text does not look right.

How to reproduce:
mc -> F2 -> F1

...
║Here is a sample mc.menu file:                                                  ║
║                                                                                ║
║        ADump the currently selected file                                       ║
║        od -c %f                                                                ║
...

But:
man mc

...
       Here is a sample mc.menu file:

       A    Dump the currently selected file
            od -c %f
...

Note

Original attachments:
@mc-butler
Copy link
Author

Changed by rimf (rimf@….ru) on Jan 9, 2017 at 5:48 UTC

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Jan 10, 2017 at 0:18 UTC

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Jan 10, 2017 at 0:25 UTC (comment 1)

(Ping. I know Trac doesn't send out emails when somebody only adds an attachment, so here's a dummy comment.)

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Jan 10, 2017 at 6:21 UTC (comment 2)

  • Priority changed from major to minor
  • Milestone changed from Future Releases to 4.8.19

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 22, 2017 at 17:51 UTC (comment 3)

  • Milestone changed from 4.8.19 to 4.8.20

I'm postponing the "milestone". Does anybody object? (The bug has been there for ages probably.)

The patch is very trivial but the reason I haven't pushed for it is because I was thinking of rewriting/rearranging the whole code there. The current code is "ad hoc" (euphemism for "awful") and has bugs in the scrolling mechanism. With a rewrite I could hopefully also write a test for the tab bug.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 22, 2017 at 19:34 UTC (comment 4)

No objections at all!

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 22, 2017 at 20:54 UTC (comment 3.5)

Replying to mooffie:

and has bugs in the scrolling mechanism. With a rewrite [...]

Getting scrolling right is really not a trivial task, especially with special formatting (e.g. hyperlinks, nroff, colors etc.) and UTF-8 (multibyte, double-wide, combining etc.) in the game.

One of the goals of the complete viewer rewrite (#3250) was to port the help viewer to that engine as well (#3396). I just never got around to that. In order to do that, there are two major features missing from the viewer: Interpreting the help format as input (this should be done on the same level as nroff is handled now), and wrapping without breaking words (should be near the current wrap/nowrap distinctions). Plus, of course, once these two are done, hook up the help viewer to actually use that.

The reason why the viewer (especially scrolling backwards) is so complicated is because it has to handle giant files and we don't want memory consumption to grow. Hence there's no caching, buffering, preformatting, whatever. The help viewer, however, only handles small pages. It's okay to render an entire help page in memory, maybe in a row-by-row GArray or such, and then scrolling up/down and such is a piece of cake. In this case it's entirely independent of the standard viewer.

If you decide to fix the help viewer, it's up to you to choose which way you go.

The current patch is, however, so small that (if indeed it works correctly) I'd vote for submitting it. It won't make your task of fixing the help viewer any easier or harder, just a tiny bit less urgent.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 22, 2017 at 21:06 UTC (comment 6)

I'm not familiar with the code and don't have the time to have a deeper look, but the patch looks sane, and if Egmont thinks it's alright as a stop-gap solution, I'd also vote to commit and continue in #3396 if that's the way forward. Mooffie, what do you think?

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 22, 2017 at 21:13 UTC (comment 7)

  • Cc set to egmont

Note: I haven't looked at the code and the patch either. I'm just generally saying that I prefer to apply tiny patches even despite a likely forthcoming rewrite.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 17:31 UTC (comment 5.8)

  • I'm moving this back to 4.8.19.
  • After doing literary and logical analysis of comment:6, comment:7 and comment:5, I have to conclude that the patch is voted for.
  • Therefore I'll soon commit the patch.

Replying to egmont:

One of the goals of the complete viewer rewrite (#3250) was to port the help viewer to that engine as well (#3396).


I'm aware of that. I didn't want to bring this up here because I'd have then had to invest time in writing a long letter explaining why I thought it wasn't a wise idea. I planned that letter for some other place and time (if at all). So I just can't explain to you here why your "Getting scrolling right is really not a trivial task" is wrong.

Here I only wanted to remove the "4.8.19" tag (as that release is near), and I had to give some explanation.

If you decide to fix the help viewer, it's up to you to choose which way you go.


It's not up to me. But let's cut the story short: let's pretend comment:3 was never ever written. It's not the time for diversions now.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 17:33 UTC (comment 9)

  • Milestone changed from 4.8.20 to 4.8.19

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 23, 2017 at 17:49 UTC (comment 8.10)

@mooffie I guess my message with porting the help viewer to the main viewer didn't get through as I intended.

I'm absolutely fine if you're not planning to migrate the help viewer to this engine. I understand that a help viewer (rendering a tiny, known-in-advance sized content) is a much simpler story than viewing potentially giant files, and that scrolling backwards is pretty easy to implement there. If you'd prefer to keep it using its own rendering engine, I'm perfectly fine with it, no explanation needed! :)

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 18:31 UTC (comment 10.11)

Replying to egmont:

@mooffie I guess my message with porting the help viewer to the main viewer didn't get through as I intended.


It's alright. I understand your concerns. And I thank you for not reading a negative tone in my reply (I was afraid of having to write yet another of those "love letters"; it's a huge time waster ;-)

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Feb 23, 2017 at 18:35 UTC (comment 12)

Aside: whatever I wrote that bore any semblance to love letters was always much shorter and less argumentative than stuff I've written to technical mailing lists... ^__^

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 18:54 UTC (comment 13)

  • Status changed from new to accepted
  • Owner set to mooffie
  • Branch state changed from no branch to approved

branch: 3754_helpviewer_tabs
[1f121b7]

(BTW, the help viewer shows a different rendering than "man mc". It's not a bug. Our viewer uses a tab sizer of 8, which is what the 'mc.hlp' file is formatted to. "man mc", OTOH, seems to use 5 as the tab size (I can see only two lines where this ruins the adjustment, but it's quite insignificant).)

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 19:06 UTC (comment 14)

(@yury: lol, you're right about love letters vs technical.)

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 19:07 UTC (comment 15)

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

Merged to master: [1db93c1]

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Feb 23, 2017 at 19:14 UTC (comment 16)

  • Status changed from testing to closed

NEWS-4.8.19 updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mooffie mooffie

Labels
area: docs Improvements or additions to documentation prio: low Minor problem or easily worked around
Milestone
4.8.19
Development

No branches or pull requests
2 participants
@mooffie @mc-butler