Ticket #3754 (closed defect: fixed)

Opened 3 months ago

Last modified 5 weeks ago

A help shows wrong

Reported by: rimf Owned by: mooffie
Priority: minor Milestone: 4.8.19
Component: documentation Version: master
Keywords: help doc tab Cc: egmont
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

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

Attachments

mc-bug-report (1.5 KB) - added by rimf 3 months ago.
3754-Help-dialog-doesnt-handle-tabs-correctly.patch (1.7 KB) - added by mooffie 3 months ago.

Change History

Changed 3 months ago by rimf

Changed 3 months ago by mooffie

comment:1 Changed 3 months ago by mooffie

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

comment:2 Changed 3 months ago by zaytsev

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

comment:3 follow-up: ↓ 5 Changed 5 weeks ago by mooffie

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

comment:4 Changed 5 weeks ago by zaytsev

No objections at all!

comment:5 in reply to: ↑ 3 ; follow-up: ↓ 8 Changed 5 weeks ago by egmont

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.

Last edited 5 weeks ago by egmont (previous) (diff)

comment:6 Changed 5 weeks ago by zaytsev

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?

comment:7 Changed 5 weeks ago by egmont

  • Cc egmont added

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.

comment:8 in reply to: ↑ 5 ; follow-up: ↓ 10 Changed 5 weeks ago by mooffie

  • I'm moving this back to 4.8.19.
  • 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.

comment:9 Changed 5 weeks ago by mooffie

  • Milestone changed from 4.8.20 to 4.8.19

comment:10 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 5 weeks ago by egmont

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

comment:11 in reply to: ↑ 10 Changed 5 weeks ago by mooffie

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

comment:12 Changed 5 weeks ago by zaytsev

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... ^__^

comment:13 Changed 5 weeks ago by mooffie

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

branch: 3754_helpviewer_tabs
changeset:1f121b76749baf5909022751933d9a284707ae51

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

comment:14 Changed 5 weeks ago by mooffie

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

comment:15 Changed 5 weeks ago by mooffie

  • Status changed from accepted to testing
  • Votes for changeset set to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged
Last edited 5 weeks ago by mooffie (previous) (diff)

comment:16 Changed 5 weeks ago by mooffie

  • Status changed from testing to closed

NEWS-4.8.19 updated.

Note: See TracTickets for help on using tickets.