nurpax / petmate

PETSCII editor with Electron/React/Redux
MIT License
179 stars 14 forks source link

off by one bug in either SEQ import or export #152

Closed nurpax closed 5 years ago

nurpax commented 5 years ago

Either the .seq export or import (or both :)) have an off by one bug. The last row of the PETSCII screen gets duplicated and looks like the image is shifted up by one row.

Here's a .petmate file that contains content on all the 25 rows. Screenshot how it's supposed to look like and how it goes wrong when imported (look at the last two rows). Also attached the exported seq file.

How it's supposed to work:

seqbug1-working

How it looks like after exporting to SEQ and then importing the SEQ:

seqbug1-after-import

Here's the petmate file and the exported seq:

attachments.zip

sixofdloc commented 5 years ago

Without looking, I'd guess that's what's happening is that scrollup (triggered when you hit the bottom/right corner) in the import isn't clearing the bottom screen row the way the c64 would. The way I see it, there are two possible useful fixes - 1. add an exception to scrollup so it doesn't fire on the last character in a file (or at least clears the bottom row when it fires), or 2. implement the expanding file patch we discussed in email?

nurpax commented 5 years ago

Yes I think there's a scroll up that shouldn't happen. But clearing the last row won't fix it as the screen has already been shifted up by one row, meaning we've lost the first row of the image.

sixofdloc commented 5 years ago

On the actual C64, the behavior would be that it scrolls the screen up by one row and leaves a blank row at the bottom. That's clearly not ideal here :) Shall I add a hook to suppress the scrollup if it's on the last character of a file? Edit: I see you're a step ahead of me. What of image expansion for larger files?

nurpax commented 5 years ago

@sixofdloc https://github.com/nurpax/petmate/pull/154 seems to fix it.

nurpax commented 5 years ago

Makes sense. I guess my change to suppress the "cursor right" on the last SEQ byte works fine then.

sixofdloc commented 5 years ago

On to expanding the screen for larger files, then?

nurpax commented 5 years ago

Yeah, that should work by just appending new rows instead of scrolling up. But I'll merge my current fix for now.