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

 
 

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

Submitted by:  David Lawrence Ramsey <dolorous>
Submitted on:  Fri 07 Jul 2017 05:42:55 AM UTC  
 
Severity: 3 - NormalStatus: Wont Fix
Assigned to: Benno Schulenberg <bens>Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

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

I meant "bug": bug #51438.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sat 22 Jul 2017 08:00:28 PM UTC, comment #10:

The opposite bug has been filed as buf #51438.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 22 Jul 2017 08:00:28 PM UTCbensOpen/ClosedOpen=>Closed
    Tue 11 Jul 2017 01:45:42 PM UTCbensAssigned todolorous=>bens
    Tue 11 Jul 2017 07:23:55 AM UTCbensStatusNeed Info=>Wont Fix
    Fri 07 Jul 2017 04:28:02 PM UTCbensStatusIn Progress=>Need Info
    Fri 07 Jul 2017 02:11:54 PM UTCdolorousAttached File-=>Added 0001-text-add-the-right-amount-of-space-with-TABS_TO_SPAC.patch, #41148
    Fri 07 Jul 2017 10:26:28 AM UTCbensStatusNone=>In Progress
      Assigned toNone=>dolorous
    Fri 07 Jul 2017 06:07:42 AM UTCdolorousAttached File-=>Added 0001-text-add-the-right-amount-of-space-with-TABS_TO_SPAC.patch, #41145
    Fri 07 Jul 2017 05:42:55 AM UTCdolorousAttached File-=>Added 0002-text-add-the-right-amount-of-space-with-TABS_TO_SPAC.patch, #41143

    Back to the top


    Powered by Savane 3.1-cleanup1