Draft: Cleaned indentation

[BrachystochroneSD]

Hello,

I would like to contribute to this project. I wasn't really satisfied with the current indentation system. I would like to have something more fluid when writing test plan.

I've taken the idea from python-mode. Major rework is done in the robot-mode-indent which now is using a robot-indent-context which determine what's the context where the current cursor is. This is easier to understand that a all-in-one function and to maintain in the future.

Since the code was getting quite consequent, I've split the package into multiple file as well. Better to structure right now.

Note that a new pull request about argument alignment will follow soon after this change (if it is accepted).

[kopoli]

Hi,

Thanks for your interest in this project and for this significant contribution !

You mentioned that you would like some more fluidity in the indentation in your test plan. Do you have some examples where you would like a different indentation than what the current functionality provides ?

Or is it about the toggling-style indentation (i.e. TAB indents, subsequent TAB dedents), as I see you set TAB and S-TAB for changing the indentation ?

[BrachystochroneSD]

Hi,

Thanks for your interest in this project and for this significant contribution !

You mentioned that you would like some more fluidity in the indentation in your test plan. Do you have some examples where you would like a different indentation than what the current functionality provides ?

Or is it about the toggling-style indentation (i.e. TAB indents, subsequent TAB dedents), as I see you set TAB and S-TAB for changing the indentation ?

Here are the two major points that was problematic for me:

• The normal return reindent the line, so either you need to use C-J or you need each time to indent correctly your already created line.

  https://github.com/kopoli/robot-mode/assets/34922853/36ca2817-f8aa-44b9-a3e5-ada8e44d01fe

• The indent for tab move the cursor to the indentation, so you need to move your cursor back to where it was

https://github.com/kopoli/robot-mode/assets/34922853/b7551f1c-c817-4362-a038-752c224cfd95

With this PR. You cursor doesn't need to move elsewhere. Here an example with the version of this PR:

https://github.com/kopoli/robot-mode/assets/34922853/3e4ae1a2-2a83-4429-8160-11ed8e91d8de

[kopoli]

Thanks!

Regarding the major problematic points (unfortunately I'm not able to view those mp4-files (Github says the file is corrupted), so I'm going by the descriptions):

1. I can reproduce this problem. When pressing RET at the end of the line, the current line is dedented just before moving to the next line. It is triggered by the enabled-by-default minor-mode electric-indent-mode. Usually I have it disabled, so I didn't notice this.

   It is a bug introduced in the commit: 5cfaf8da2ce3484a81c1249726172be9056e4a6e Add nested indenting for control structures

   It needs to be fixed.

2. Moving back to indentation when pressing TAB is something this major-mode has done since the earliest versions. Therefore it is established behaviour. I can see that it would be a good feature addition to retain the relative cursor position when indenting. In my opinion that should be opt-in at start. I.e. to have a defcustom that enables it, but it would be nil by default.

Regarding the pull-request itself: I appreciate your efforts in discovering the above issues, and laud finding the time to rewrite the indentation functionality.

However, in my opinion it is too big a change. It increases the overall size of the code by half, and more than triples the size of the indentation code (from 80 lines to 270 lines).

Upon accepting this change I would have to maintain it indefinitely. IMO maintaining less code is easier than maintaining more code. One example of this is the rather large change to support control structure indentation. There were a lot of bugs during the development (#2) and, as you can attest with the problematic point 1., bugs are still being found from it. Tripling the size of that functionality will likely generate a lot more bugs in the years to come.

Therefore I would prefer that the changes to this package would be smallish and incremental.

Are there other problematic points in the current indentation functionality ?

[BrachystochroneSD]

I understand your point, this PR could be optimized.

But from my experience, the number of lines is clearly not proportional to the maintenance difficulty. A cleaner code is better than a smaller code. The fact that this PR contain more line is because :

1. the function to get the context is detached from the function to calculate the indentation. And this is to permit easier addition in the future in case of new context/indentation
2. Because I added much more context than the one previously checked in your code, I choose to have more distinct contexts even if the resulting indentation is the same because:
   1. It's easier to understand when you see
3. Because there is lots of documentation lines

The difficulty you've encountered in the past by having to implement the control structure would've been trivial by using the current implementation of the indentation calculation system proposed in this PR. Only a new context with its regular expression need to be added, giving it's priority in the robot-indent--get-context and its resulting indentation. basically:

     ;; After block start
     ((or
       (robot-indent--check-line robot-dedenter-block-regex -1) ;; Check previous line with dedenter block regex, like "ELSE"
       (robot-indent--check-line robot-block-start-regex -1)) ;; check previous line with block start regex, like "IF ..."
      (cons :after-block-start previous-line-start))
     ;; At block end
     ((robot-indent--check-line robot-block-end-regex) ;; Check if the current line start with block-end regex, so "END"
      (cons :at-block-end previous-line-start))

and add the context property name in the pcase where we add one indentation level ( next to :after-defun) and decrement one at block end in the robot-indent--calculate-indentations

      (`(,(or :after-defun       
              :after-block-start)                                                        ;; If :after-defun or :after-block start...
         . ,start)
       (goto-char start)                                                                
       (+ (current-indentation) robot-mode-indent-level))        ;; Increment the current indentation by the default indent-level
      (`(,(or :at-block-end                                                            ;; if :at-block-end or :at-dedenter-block
              :at-dedenter-block)
         . ,start)
       (goto-char start)
       (- (current-indentation) robot-mode-indent-level))         ;; decrement the current-indentation by the default indent-level

This is much simpler to maintain than having to change the whole function itself. I will check if I can optimize this PR I must admit that it could be better. Feel free to propose any changes as well.

Also, I can participate to the maintenance of this project as well. I will use this robot-mode for a while since it's the automation solution chosen in my company. And I am full time on it.

I'll put this PR as draft, Let me see if I can make it easier more optimized.

[BrachystochroneSD]

Note also, multiples indentations (levels 0 and 1) should only be allowed in some context. For example, cycling between 0 and 1 indentation is only permitted in those lines given my implementation of the indentation process:

Test Case 1
    [Arguments]    ${arg1}    ${arg2}  # no cycle
    ...            ${arg3}             # no cycle

    Keyword    arg1    arg2            # no cycle 
    This is something                  # cycle permitted
    IF    ${arg1=="hello"}             # no cycle 
        This is something good         # no cycle 
    ELSE                               # no cycle
        This is other thing            # no cycle
    END
    Hello world                        # cycle permitted

Which could potentially permit the electric-indent for those who like to use this feature, only inhibit it when at these contexts.

Moving back to indentation when pressing TAB is something this major-mode has done since the earliest versions. Therefore it is established behaviour. I can see that it would be a good feature addition to retain the relative cursor position when indenting. In my opinion that should be opt-in at start. I.e. to have a defcustom that enables it, but it would be nil by default.

What's the benefit of having your cursor going back to indentation? All mode that I know (python-mode, gdscript-mode, yaml-mode) that allow multiple indentations levels go back to the cursor initial position after indenting, only following the indentation when the cursor is within the indentation.

Like the function used in this PR:

(defun robot-indent-line (&optional previous)
  (let ((follow-indentation-p
         ;; Check if point is within indentation.
         (and (<= (line-beginning-position) (point))
              (>= (+ (line-beginning-position)
                     (current-indentation))
                  (point)))))
    (save-excursion
      (indent-line-to
       (robot-indent--calculate-indentation previous)))
    (when follow-indentation-p
      (back-to-indentation))))

But if you want, indeed a defcustom can be created and add this or statement at this line:

 (when (or follow-indentation-p robot-indent-follow-indentation)
      (back-to-indentation))))

[kopoli]

I added #8 that may address the mentioned problematic points 1. and 2. as far as I understand them.

The fix for 1. was about 3 lines, and adding the feature for 2. was 10 lines. These are in my opinion easy maintenance.

Which could potentially permit the electric-indent for those who like to use this feature, only inhibit it when at these contexts.

With the fix for 1. inhibiting the electric-indent-mode shouldn't be necessary.

What's the benefit of having your cursor going back to indentation?

It was a design decision early in the development. Unfortunately I don't remember the exact reasoning. The main point here is that IMO it is extremely valuable to remain backwards compatible when updating Emacs packages, so they won't break people's use cases.

A relevant example I recall is the electric-indent-mode being set default (basically swapping the functionality for C-m and C-j) in Emacs in some version, and the shenanigans that ensued when things worked differently than before.

[BrachystochroneSD]

I understand that you feel more comfortable with your legacy code. I'll close this PR if you manage to fix the two major issue mentioned earlier (and fixed in this PR).

Thanks for the consideration.

[BrachystochroneSD]

Closing since #8 will fix issues already fixed by this PR.