robhagemans / pcbasic

PC-BASIC - A free, cross-platform emulator for the GW-BASIC family of interpreters
http://www.pc-basic.org
Other
393 stars 48 forks source link

PRINT creates CR when at screen edge, but it should not. #168

Closed JDoucette closed 2 years ago

JDoucette commented 2 years ago

Bug report

Problem If PRINT prints data that touches the screen edge, PC-BASIC creates a CR, but GW-BASIC does not.

Steps

  1. Run the program in PC-BASIC (1.2 or 2.0, Tandy mode or not)
  2. See there is a space between LAST LINE and the previous 40-char width PRINT.
  3. There should be no space.

Program

10 WIDTH 40
20 PRINT "12345678901234567890123456789012345678"
30 PRINT "123456789012345678901234567890123456789"
40 PRINT "1234567890123456789012345678901234567890"
50 PRINT "LAST LINE"

Crash log None.

Notes PC-BASIC version: 1.2 and 2.0
Operating system version: Windows 10

PC-BASIC-1 2 PC-BASIC-2 0 TANDY-GW-BASIC-3 2-DOSBOX

rbergen commented 2 years ago

I think this is caused by this line in /pcbasic/basic/devices/formatter.py and the one that follows it.

They specifically add a second line break if a newline is requested and a character has been printed in the last column on the screen; that being indicated by Formatter._console.overflow being True. Removing the two lines would fix this. However, it looks to me like they were deliberately added to insert the extra newline, and I don't know what removing the lines would break.

rbergen commented 2 years ago

After giving it some thought, it may be that the intended functionality would be achieved by replacing lines 41 to 44, that being

        if newline:
            if self._console and self._console.overflow:
                self._output.write_line()
            self._output.write_line()

with the following code:

        if newline or (self._console and self._console.overflow):
            self._output.write_line()
robhagemans commented 2 years ago

Any change to this is really going to need a lot of testing, this is a fundamental part of the code and everything depends on everything. Unfortunately (1) a lot of this is difficult to implement as an automated test so needs to run through a number of example programs and be checked visually and (2) not all tests pass in any case .(iirc there's 28 test failures) so you need great care to avoid regression...

On Tue, 30 Nov 2021, 07:32 Rutger van Bergen, @.***> wrote:

After giving it some thought, it may be that the intended functionality would be achieved by replacing lines 41 https://github.com/robhagemans/pcbasic/blob/0e0a758d7dca91daf6bfe8c6111218f46967a9ae/pcbasic/basic/devices/formatter.py#L41 to 44, that being

    if newline:
        if self._console and self._console.overflow:
            self._output.write_line()
        self._output.write_line()

with the following code:

    if newline or (self._console and self._console.overflow):
        self._output.write_line()

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/robhagemans/pcbasic/issues/168#issuecomment-982359334, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB25REWGSOZ44534HM6LK73UOR4XVANCNFSM5IGAZJ2A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rbergen commented 2 years ago

This is exactly why I'm sharing thoughts and not opening a PR for this. Yet. :)

rbergen commented 2 years ago

I was wondering, is there any text (documentation, existing test code, ...) I could read that would tell me what tests would need to be performed to verify the correct operation of screen output routines?

robhagemans commented 2 years ago

Not really, sorry... There are the automated tests however in he test directory that cover at least part of what's needed

On Wed, 1 Dec 2021, 08:18 Rutger van Bergen, @.***> wrote:

I was wondering, is there any text (documentation, existing test code, ...) I could read that would tell me what tests would need to be performed to verify the correct operation of screen output routines?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robhagemans/pcbasic/issues/168#issuecomment-983397642, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB25RET6F4IY7MIXKMO5VS3UOXK5RANCNFSM5IGAZJ2A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rbergen commented 2 years ago

I understand. I'll have to give this some thought. On the one hand I'm very willing to put in the time and effort to improve PC-BASIC, but I really do not want to introduce new and hard-to-diagnose bugs in such a critical part of the application.

robhagemans commented 2 years ago

Yes, I'll have a look at it anyway, but I'm not sure when.

At the very least I'll turn the bug example into an additional test and see if your proposed change breaks anything else I know of

On Wed, 1 Dec 2021, 16:19 Rutger van Bergen, @.***> wrote:

I understand. I'll have to give this some thought. One the one hand I'm very willing to put in the time and effort to improve PC-BASIC, but I really do not want to introduce new and hard-to-diagnose bugs in such a critical part of the application.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robhagemans/pcbasic/issues/168#issuecomment-983804521, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB25RESCOF4TN2D7Q5YC4G3UOZDKRANCNFSM5IGAZJ2A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rbergen commented 2 years ago

I've done some exploratory testing with the change I suggested in place, and so far everything I've tried (line breaks, line editing, line deletion, etc.) works as I would expect it.

robhagemans commented 2 years ago

So, if I run this in my copy of DOSBox I do actually get the additional newline. Which probably explains why the specific line in the code is there. Has anyone tried this on real hardware?

robhagemans commented 2 years ago

This is what I see in VirtualBox, MS-DOS 6.22 with GW-BASIC 3.23:

vbox_msdos_gwbasic_extra_cr

A test on real hardware would be more authoritative, but as it stands I'm inclined to think this is not a bug in PC-BASIC but in that version of DOSBox. Unless this is dependent on the particular video card?

rbergen commented 2 years ago

That's interesting, indeed. Personally, I'm happy to reject my own PR because of the VirtualBox evidence. Just to get my facts straight, what version of DOSBox are you using?

robhagemans commented 2 years ago

0.74, but they all seem to have that version number...

DualBrain commented 2 years ago

To quote one of the many GW-BASIC manuals I have...

If a comma, semicolon, or SPC or TAB function ends an expression list, the next PRINT statement begins printing on the same line, accordingly spaced. If the expression list ends without a comma, semicolon, or SPC or TAB function, a carriage return is placed at the end of the lines (GW-BASIC places the cursor at the beginning of the next line).

A carriage return/line feed is automatically inserted after printing width characters, where width is 40 or 80, as defined in the WIDTH statement. This results in two lines being skipped when you print exactly 40 (or 80) characters, unless the PRINT statement ends in a semicolon.

rbergen commented 2 years ago

A carriage return/line feed is automatically inserted after printing width characters, where width is 40 or 80, as defined in the WIDTH statement. This results in two lines being skipped when you print exactly 40 (or 80) characters, unless the PRINT statement ends in a semicolon.

I've now found various digital copies of the GW-BASIC user manuals that say exactly that. I'm sufficiently convinced that PC-BASIC's current behaviour is in fact correct. I'll close #170.

robhagemans commented 2 years ago

Closing as not a bug. Added the example code in this issue as a test case, commit 2f77b44b