dykstrom / basic-mode

Emacs major mode for editing BASIC code
GNU General Public License v3.0
7 stars 10 forks source link

Feature: Renumber #5

Closed pederkl closed 6 years ago

pederkl commented 6 years ago

Hm. Maybe it's not a problem creating a new PR anyway. Just be aware that the first couple of commits here are the subject of PR#4, which needs to be committed first. (well, it's mostly independent features, but both use the new function basic-current-line-number, which was introduced in PR#4.)

Anyway, here it is, the renumbering feature. Works by two passes - the first scans the whole buffer for jumps, and puts markers on them, associated with the target line numbers. The second pass does the renumbering, and checks if the line being renumbered was the target of any jumps. In that case, the list of markers for that target is traversed and updated.

The only need I have for Basic is for nostalgia, playing with Commodore 64 emulation in Vice. The basic version in C64 does not have the "on x goto a,b,c" construct. In fact I was not aware of such a construct until earlier today. Consequently, the first iteration of this feature does not support those jump constructs.

I believe the basic algorithm I described above is still applicable for on.. jumps, but the scanning pass needs a smarter parser to deal with lists of numbers after goto/gosub, and placing markers after each number.

pederkl commented 6 years ago

I just remembered that "if x=y then 300" is another form of jump that needs updating. 035d4ac does not include that. I'll fix that later, should be simple to adjust the jump-finding regexp.

dykstrom commented 6 years ago

The Sinclair Basic I used back in the days did not have the on-goto feature either that I can remember, but I thought basic-mode should be compatible with QuickBasic, which seems to be a popular dialect among old Basics. I have not gone through the renumbering algorithm in detail, since it is a bit complicated, but I have done some happy testing. I like that you can renumber just the region, if you want. But I found two things that I think could be improved.

  1. Sometimes the last row in the buffer is not renumbered. I have not been able to determine the exact conditions for this, but it only happens to the last line.

  2. If you have lines with only some blank characters on them (not completely empty lines), they are assigned a line number too. I think it would be better if they remained as they were.

Otherwise I think the renumbering feature is very nice! :-)

dykstrom commented 6 years ago

What do you think about a possibility to customize if lines without line numbers (not empty lines, but lines with code) should be assigned a line number or not during renumbering? At the moment all lines get a line number regardless of if they had one before or not, and that may not be what you want if you are using a Basic where line numbers are optional.

pederkl commented 6 years ago

On Thu, Nov 16 2017 at 20:06, Johan Dykström wrote:

  1. Sometimes the last row in the buffer is not renumbered. I have not been able to determine the exact conditions for this, but it only happens to the last line.

Yeah, there's something not quite right about the "loop over all lines"-part.

  1. If you have lines with only some blank characters on them (not completely empty lines), they are assigned a line number too. I think it would be better if they remained as they were.

I agree. I thought about this several times, but it's always slipped my mind by the time I sat down to code.

I'll see if I get some time during the weekend to fix these.

Otherwise I think the renumbering feature is very nice! :-)

Thanks! :)

pederkl commented 6 years ago

On Fri, Nov 17 2017 at 07:42, Johan Dykström wrote:

What do you think about a possibility to customize if lines without line numbers (not empty lines, but lines with code) should be assigned a line number or not during renumbering?

Probably a good idea. I can take a look at how difficult that would be to implement.

At the moment all lines get a line number regardless of if they had one before or not, and that may not be what you want if you are using a Basic where line numbers are optional.

True. But if you are using a basic where line numbers are mandatory, the current behaviour allows you to skip writing line numbers while you are coding, except using them as jump labels, and then just renumber the whole thing at the end.

So both models are valid, and should be supported.

dykstrom commented 6 years ago

I agree, both models should be supported if possible.

pederkl commented 6 years ago

Wasn't really difficult at all :)

dykstrom commented 6 years ago

I have tested your changes, and they seem to work fine. But when testing, I found some new issues for you to fix. :)

  1. When renumbering a region, all lines below the region are also renumbered.
  2. If you have "turned off" line numbers by setting basic-line-number-cols to 0, but have the default value of basic-renumber-unnumbered-lines (t), and type C-c C-r, you get some garbage characters in position 1 of all lines. One could argue that the user is stupid if doing so, but we could also stop the user from doing the stupid thing by checking basic-line-number-cols or in some other way.
pederkl commented 6 years ago

Gah. no. 1 is just a stupid thinko, sorry. Fix coming up.

I don't understand why you get garbage in the case you describe in no. 2, but I see the same. I also agree we should protect the user from this scenario. Patch coming up, fixing both auto-numbering and renumbering. God helg. :)

pederkl commented 6 years ago

Bugger. Forgot that this branch had an old version of the auto-number function.

pederkl commented 6 years ago

And I can't even spell the name of the function correctly in the merge message. It's obviously getting to late to code.

There is still something funky going on with the auto-numbering, but I'm unsure if that is my doing or not. If basic-line-number-cols is 0, the indentation seems weird when I press enter on a line starting with a number.

dykstrom commented 6 years ago

I think that problem was there before. If you set basic-line-number-cols to 0 and still start your lines with line numbers, the indentation does not work. I should fix that after merging all of your changes. I will try to test your latest fixes this evening, and maybe merge too.

dykstrom commented 6 years ago

Now I merged the renumbering PR as well. It seems to work as intended. I fixed some minor checkdoc errors and updated the general documentation of the package. If you want to have a go at fixing the on-goto problem, you can create a new PR. Otherwise, I will fix it some time in the future.

pederkl commented 6 years ago

Thanks!