bugGNU nano - Bugs: bug #51408, indenting via M-} can add a wrong...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #51408: indenting via M-} can add a wrong number of spaces with --tabstospaces

Submitter:  David Lawrence Ramsey <dolorous>
Submitted:  Fri 07 Jul 2017 05:42:55 AM UTC
   
 
Severity:  3 - Normal Status:  Wont Fix
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Fri 08 Dec 2017 08:20:00 PM UTC, comment #11: 

I meant "bug": bug #51438.

Benno Schulenberg <bens>
Group administrator
Sat 22 Jul 2017 08:00:28 PM UTC, comment #10: 

The opposite bug has been filed as buf #51438.

Benno Schulenberg <bens>
Group administrator
Tue 11 Jul 2017 07:23:55 AM UTC, comment #9: 

So, we've agreed that the bug is the opposite one: "*without* --tabstospaces, indenting via M-} adds a non-fixed amount of whitespace".

Benno Schulenberg <bens>
Group administrator
Fri 07 Jul 2017 07:37:33 PM UTC, comment #8: 

Okay; I figured as much.

As for undoing a mixed unindent, the comment code saves the comment string in u->strdata in case the filename (and hence the associated comment) changes.  I'm still trying to wrap my head around the undo code, but I suppose the indent code, when it's hooked up to undo/redo, could save the tab's worth of characters that gets added to or removed from the line in u->strdata, and the undo/redo code could use it to make sure that all indents are saved and restored properly.  (This would also need to be done in case the TABS_TO_SPACES flag changes.)

But I should really take my comments to the other bug now :)

David Lawrence Ramsey <dolorous>
Group Member
Fri 07 Jul 2017 06:05:01 PM UTC, comment #7: 

For unindenting, the type of whitespace should not matter, as long as all selected lines can be unindented by a full tab's worth.  (If not all lines can be unindented, M-{ should do nothing.)

Not caring about the type of whitespace makes it possible to quickly normalize the indentation of a piece of text to all spaces or all tabs (not considering partial indents) by selecting the text, doing M-{ a few times, and then doing M-} the required number of times.  That is quicker, I think, than having to resort to search-and-replace.

(The other way around: caring about the type of whitespace would frustrate the user when he/she does not realize there is mixed whitespace in the file.  Very annoying.)

Oof... but that will mean that undoing such a mixed unindent will not restore the situation exactly as it was before, because all the undo will do is add back a tab (or a tab's worth of spaces) to all the lines that were selected.  Hmmm...  Can't be helped.  It's better than the current state: no undo available at all.

Benno Schulenberg <bens>
Group administrator
Fri 07 Jul 2017 04:53:31 PM UTC, comment #6: 

After some thought, I'm in favor of the beginning for both indent and unindent.  However, I thought I'd fix the existing behavior before changing it, which is why I made this patch first.

And I'm looking into cleaning up the function a bit before anything else, as I don't remember what I was thinking when I designed it to use the cols/-cols method to determine whether to indent or unindent.

The one real question I have relates to unindent.  Currently, it doesn't care what whitespace it removes, as long as it takes up the same number of columns as an indent would (the --tabstospaces option makes the contents of a given indent vary, after all).  And your comments in the other bug indicate that nano should ignore partial indents, which I'd be in favor of too.

So... what should your ideal unindent do if a block of text has full indents, but they're of different types?  Say the block we've highlighted contains two lines: the first is indented with a tab, the second is indented with a tab's worth of spaces.  Should unindent work on both lines, or should it refuse unless both lines have not just full indents, but the same type of full indents (both tabs, or both tabs' worth of spaces)?

And yes, this should definitely go into that other bug, and I'll put it in there after I get an answer to this.  I just need to know how to proceed.

David Lawrence Ramsey <dolorous>
Group Member
Fri 07 Jul 2017 04:28:02 PM UTC, comment #5: 

The question is: should indenting add a tabsize worth of whitespace at the end of the line's leading whitespace, or at the beginning?  The current method is weird, because it will add a tab after spaces (when tabstopaces is not set), which a) is bad form, and b) it doesn't preserve the relative indentation if some lines are indented by four spaces and others by a tab, for example.

If I would apply your patch, I would effectively strengthen the "wrong" method of indenting.  I think that instead the opposite patch is needed, so that, also when tabstospaces is unset, an M-} will increase the visual identation of all selected lines by a full tab width.

(A secondary reason why I want it that way is that it is far easier to undo: just add or remove again one tab (or the corresponding number of spaces) to or from the head of each selected line (unless the line is empty), similar to what is done for commenting/uncommenting.  When wanting to undo the current method of indenting, and the selected lines are each indented by a different number of spaces, how are we going to remember how many spaces were added to which line (when tabstospaces is set)?  That requires a lot of bookkeeping.)

In short: I'm going to hold off on this patch.

Benno Schulenberg <bens>
Group administrator
Fri 07 Jul 2017 02:11:54 PM UTC, comment #4: 

Updated version attached.  And I'll definitely look at that other bug.

(file #41148)

David Lawrence Ramsey <dolorous>
Group Member
Fri 07 Jul 2017 11:03:22 AM UTC, comment #3: 

By the way, maybe have a look at bug #47684 too, and maybe comment there how /you/ think that indenting/unindenting should work when there are partial indents.

Benno Schulenberg <bens>
Group administrator
Fri 07 Jul 2017 10:26:28 AM UTC, comment #2: 

Patch doesn't apply.  You changed some braces in the meantime.  Please provide an updated patch against master.

Benno Schulenberg <bens>
Group administrator
Fri 07 Jul 2017 06:07:42 AM UTC, comment #1: 

Forget the last patch; the calculation was off since it relied on the current x position.  The attached new patch properly calculates the number of spaces to indent per line when multiple lines are selected.

(file #41145)

David Lawrence Ramsey <dolorous>
Group Member
Fri 07 Jul 2017 05:42:55 AM UTC, original submission:  

Indenting via Meta-} is supposed to add a tab or the equivalent to the indentation already on a line.  However, when the --tabstospaces option is used, Meta-} always adds tabsize's worth of spaces to that place, instead of adding the number of spaces that pressing Tab would add.  This can be reproduced as follows:

1. Run nano with a new file:

./nano --ignore

2. On the first line, press Space twice, and then press Meta-}.  The line will now contain two spaces and a tab, for a total of 8 columns, not counting the newline.

3. Press Enter to make a second line.

4. Press Meta-Q to turn on the --tabstospaces option.

5. On the second line, press Space twice, and then press Meta-}.  The line will now contain two spaces and eight spaces, for a total of 10 columns, not counting the newline.

The Meta-} in step 5 should have added six spaces, not eight, in order to account for what a tab would take up relative to the current cursor position.  The attached patch fixes this.

David Lawrence Ramsey <dolorous>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by dolorous (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-22 bens Open/ClosedOpen Closed
    2017-07-11 bens Assigned todolorous bens
    2017-07-11 bens StatusNeed Info Wont Fix
    2017-07-07 bens StatusIn Progress Need Info
    2017-07-07 dolorous Attached File- Added 0001-text-add-the-right-amount-of-space-with-TABS_TO_SPAC.patch, #41148
    2017-07-07 bens StatusNone In Progress
        Assigned toNone dolorous
    2017-07-07 dolorous Attached File- Added 0001-text-add-the-right-amount-of-space-with-TABS_TO_SPAC.patch, #41145
    2017-07-07 dolorous Attached File- Added 0002-text-add-the-right-amount-of-space-with-TABS_TO_SPAC.patch, #41143

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code