perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

perldebug: clarify the readline support

Open x-yuri opened this issue 2 years ago • 2 comments

Also I don't see any traces that Term::ReadKey is used (rg ReadKey /usr/share/perl5 /usr/lib/perl5). I'm running perl-5.36.0 under Linux. And it doesn't seem like installing it makes any difference. Is it needed only under Windows and for it to get used you need Term::ReadLine::Perl5? But the former is a dependency of the latter, so that should be handled automatically. Which means under Windows Term::ReadLine::Perl5 is preferred over Term::ReadLine::Gnu?

In other words a couple of corrections might be needed. Another one: Perl -> Perl5.

x-yuri avatar Sep 16 '22 16:09 x-yuri

Also I don't see any traces that Term::ReadKey is used (rg ReadKey /usr/share/perl5 /usr/lib/perl5). I'm running perl-5.36.0 under Linux. And it doesn't seem like installing it makes any difference. Is it needed only under Windows and for it to get used you need Term::ReadLine::Perl5? But the former is a dependency of the latter, so that should be handled automatically. Which means under Windows Term::ReadLine::Perl5 is preferred over Term::ReadLine::Gnu?

In other words a couple of corrections might be needed. Another one: Perl -> Perl5.

I'm confused by your comment. If Term::ReadKey is indeed not used, then why did you not delete the mention of it in your patch?

Also, is this pull request at all related to the issue you filed today: https://github.com/Perl/perl5/issues/20307 ?

jkeenan avatar Sep 16 '22 19:09 jkeenan

I'm confused by your comment. If Term::ReadKey is indeed not used, then why did you not delete the mention of it in your patch?

Well, I made a change. But than a couple of thoughts came to mind. And while I'm certain about the change, I'm not sure about the rest:

  1. Term::ReadKey might be needed only on Windows:

    Version 2.30.01: Added handling of arrows, page up/down, home/end, insert/delete keys under Win32. These keys emit xterm-compatible sequences. Works with Term::ReadLine::Perl.

    In that case for it to be used you need Term::ReadLine::Perl5. But Term::ReadLine::Perl5 depends on Term::ReadKey anyway. As such the latter doesn't need to be mentioned. Unless I'm missing something.

  2. Which makes me think, if Term::ReadLine::Perl5 was created to overcome issues with Term::ReadLine::Gnu on Windows. Another possibility to improve the docs.

  3. Shouldn't Term::ReadLine::Perl be changed to Term::ReadLine::Perl5? Term::ReadLine::Perl seems to exist, but old and maybe abandoned.

I think we could discuss here if any of the above assumptions are true and should be implemented in this PR. Or I'm open to your other suggestions.

Also, is this pull request at all related to the issue you filed today: https://github.com/Perl/perl5/issues/20307?

No, not related. I don't have a solution for that one in mind.

x-yuri avatar Sep 17 '22 00:09 x-yuri

Term::ReadLine::Perl (no 5) uses Term::ReadKey to read the terminal size, falling back to an ioctl(), the 5 version always uses Term::ReadKey to read the terminal size.

Shouldn't Term::ReadLine::Perl be changed to Term::ReadLine::Perl5? Term::ReadLine::Perl seems to exist, but old and maybe abandoned.

The plain Perl version is passing on CPAN testers, and on many platforms, perhaps the author doesn't think it needs an update.

I think the change adds a little clarity (you don't need all of the Term::RL::* modules, just one).

tonycoz avatar Oct 18 '23 04:10 tonycoz

@x-yuri, more than a year has elapsed since there last was activity in this pull request. No one has spoken up in favor of this request, so I'm going to label the ticket 'Closable?'. I am self-assigning it for the purpose of closing it within 1 or 2 weeks unless there is a strong argument for keeping it open.

jkeenan avatar Nov 22 '23 22:11 jkeenan

I think the change as is is an improvement.

Installing Term::Key is desirable so I think leaving the suggestion to install it is fine.

tonycoz avatar Nov 22 '23 23:11 tonycoz

I think this should be merged. @tonycoz spoke up in favor of it, but @jkeenan says no one has. I don't understand

khwilliamson avatar Dec 05 '23 12:12 khwilliamson

I think this should be merged. @tonycoz spoke up in favor of it, but @jkeenan says no one has. I don't understand

@khwilliamson, in his Oct 18 comment @tonycoz expressed some doubts about the patch. Subsequently I reviewed the ticket, did not find any unambiguous support for the patch and applied the "Closable?" label. Tony then clarified that he was in favor of the patch. At that point I removed the "Closable?" label. So, if both you and Tony support the patch, you can merge.

jkeenan avatar Dec 05 '23 12:12 jkeenan