Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

5.1.1 : omitted indentation when pasting with the editor #16159

Closed
texadactyl opened this issue Aug 5, 2021 · 19 comments · Fixed by #16164 or #16214
Closed

5.1.1 : omitted indentation when pasting with the editor #16159

texadactyl opened this issue Aug 5, 2021 · 19 comments · Fixed by #16164 or #16214

Comments

@texadactyl
Copy link

Installed 5.1.1 and reset to factory defaults.
Then, I created a small function:

def f():
    x = 1
    y = 2

I wanted to add another assignment just after x = 1. So, I copied that entire line and pasted it. This was the result:

def f():
    x = 1
x = 1
    y = 2

Uninstalled 5.1.1 and installed 5.0.5. Then, I reset to factory defaults and created the same small function:

def f():
    x = 1
    y = 2

Then, I copied that entire x = 1 line and pasted it. This was the result:

def f():
    x = 1
    x = 1
    y = 2

IMO, the 5.0.5 editor behaved correctly but the 5.1.1 editor neglected to paste in the indentation.

@ccordoba12
Copy link
Member

Hey @texadactyl, thanks for reporting. I can reproduce this error.

@impact27, could you take a look at this?

@impact27
Copy link
Contributor

impact27 commented Aug 5, 2021

The expected behaviour is that it will correct the indentation to where your cursor is. If you add a line below x = 1 you should get (| is the cursor):

def f():
    x = 1
    |
    y = 2

If you then paste you should get:

def f():
    x = 1
    x = 1
    y = 2

If instead you want to de-dent, you should, from:

def f():
    x = 1
    |
    y = 2

Press delete:

def f():
    x = 1
|
    y = 2

Then paste:

def f():
    x = 1
x = 1
    y = 2

The new logic tries pastes the code in the clipboard on the indentation your cursor is.

@ccordoba12
Copy link
Member

@impact27, if the first line a user is trying to paste contains some indentation, I think we should preserve it, and adjust the indentation of all other lines according to that.

What do you think?

@texadactyl
Copy link
Author

texadactyl commented Aug 5, 2021

Folks are used to the spyder editor as it was through 5.0.5. The 5.1.1 editor-paste behaviour will require a lot of users to re-think the way they do copy/cut and paste of large blocks of code. Small segments are no big deal.

I do not copy/cut and paste a large block of code spanning multiple lines the way you showed. I'd much rather do a large collection of lines in a line-oriented fashion (cursor on first column). Less chance of a mess. If I need to indent or unindent afterwards, I can plan for that.

However, there must be some benefit for this change or you would not have made it. I'll try to figure that out.

Not trying to give you a load of re-do.

@impact27
Copy link
Contributor

impact27 commented Aug 6, 2021

What should happen in the case when pasting [4 spaces]x = 1:

def f():
    x = 1
    |
    y = 2

I think:

def f():
    x = 1
    x = 1
    y = 2

if more helpful than:

def f():
    x = 1
        x = 1
    y = 2

But the case in this issue is more debatable. Maybe we could only de-dent if it would result in a non-valid code?

@impact27
Copy link
Contributor

impact27 commented Aug 6, 2021

I think a sound logic would be for pasting to do:

  1. When pasting multiline text, it keeps the following lines at the same indentation as the first line. Exception: The following lines maximum de-dent is the smallest indentation in the clipboard.
  2. For python code, it tries to avoid creating over-indented lines (created because of auto-indentation). To achieve this, it will de-dent until reaching a valid indentation. The resulting indentation can not be smaller than either:
    • The current cursor position
    • The copied indentation

Opened a PR #16164

@impact27
Copy link
Contributor

impact27 commented Aug 6, 2021

For example, pasting [4 spaces]x = 1 would result in:

def f():
    x = 1
|
    y = 2
def f():
    x = 1
    x = 1
    y = 2

or:

def f():
    x = 1
    |
    y = 2
def f():
    x = 1
    x = 1
    y = 2

or:

def f():
    x = 1
        |
    y = 2
def f():
    x = 1
         x = 1
    y = 2

Pasting x = 1:

def f():
    x = 1
|
    y = 2
def f():
    x = 1
x = 1
    y = 2

or:

def f():
    x = 1
    |
    y = 2
def f():
    x = 1
    x = 1
    y = 2

or:

def f():
    x = 1
        |
    y = 2
def f():
    x = 1
         x = 1
    y = 2

@impact27
Copy link
Contributor

impact27 commented Aug 6, 2021

@ccordoba12 Do you think this is reasonable? Alternatively we can turn off the point 2 completely.

@sphh
Copy link
Contributor

sphh commented Aug 11, 2021

This new behaviour also puzzled me and it took me some time to figure out the logic behind it.

#16159 (comment) sounds logic to me.

There might be another solution, based on my current workflow:

  1. I paste some lines
  2. I select those pasted lines
  3. I indent them with TAB and SHIFT-TAB to the position I want them to be in

So my suggestion would be to paste them according to any of the two policies but select the pasted lines automatically after pasting. That way you are immediately able to adjust the indentation level after pasting.

What do you think?

@impact27
Copy link
Contributor

The problem with selecting is that if you paste and then write it would remove the lines. The challenge is to do something usefull but try to avoid messing up with the expectations of the user. I don't think the user would expect to have the pasted text selected, even it it could be useful.

@sphh
Copy link
Contributor

sphh commented Aug 11, 2021

I also just noticed a problem: If you past text inside some existing text, it might not be such a good idea, if it becomes selected.

Hence I would like to update my proposal, to select it only, if the selected text contains at least one newline character. Then only blocks would get selected after pasting.

Yes, it is impossible to make it right for everyone in every use case!!

Alternatively, if that is not an option, a way to select the just-pasted content with a hot key would improve the workflow massively …

@texadactyl
Copy link
Author

texadactyl commented Aug 11, 2021

Your users have a variety of habits and concepts of convenience. Cannot please them all. If it were me doing this, I would emphasize safety and consistency.

@sphh
Copy link
Contributor

sphh commented Aug 11, 2021

Yes, indeed!

I believe that @impact27's suggestion is a good solution. If that is not acceptable there is always my suggestion, either with autoselecting the pasted block or adding a keyboard hotkey to select it after pasting, to be able to adjust the indentation quickly (this hotkey can also be added in addition to @impact27's solution and might improve it even further!)

@ccordoba12
Copy link
Member

@ccordoba12 Do you think this is reasonable?

Yeah, that sounds quite reasonable to me.

@sphh
Copy link
Contributor

sphh commented Aug 13, 2021

If I apply the changes from 48065f0 (I double checked it) I get this error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/spyder/plugins/editor/widgets/codeeditor.py", line 2836, in paste
    max_dedent = min(
ValueError: min() arg is an empty sequence

What I did?

  1. I started Spyder, which opened a couple of files, because they belonged to a project I was working before.
  2. In the file:
#!/usr/bin/env python3
"""
Blabla

"""

import sys
import argparse
import ast

try:
    import colored_traceback
except ImportError:
    pass
else:
    colored_traceback.add_hook()

...

I positioned the cursor at the start of the line import coloured_traceback and selected that line with Shift+LineDown.
3. I pressed Ctrl+C, followed immediately by Ctrl-V
and got the above error.

@ccordoba12
Copy link
Member

I can't reproduce this error. Could you upload an animated gif that shows exactly how to generate it?

@sphh
Copy link
Contributor

sphh commented Aug 13, 2021

Just in case, I messed something up, I copied this file (https://raw.githubusercontent.com/spyder-ide/spyder/48065f03dc7edb04093425032f9c69f0a26dda25/spyder/plugins/editor/widgets/codeeditor.py) over the existing codeeditor.py.

In the following screencast I use Ctrl-X and Ctrl-V after having selected the line:
https://user-images.githubusercontent.com/4856462/129401521-0c276e40-c1e2-409d-aa14-e52ebeb03055.mp4

@sphh
Copy link
Contributor

sphh commented Aug 13, 2021

I might have an idea, what is going on:

The selected text is

    import colored_traceback

(note the empty line at the end).

first_line_selected, *remaining_lines = (text + eol_chars).splitlines()
splits this into

["    import colored_traceback", ""]

remaining_lines hence becomes [""]

Finally

if len(remaining_lines) > 0 and len(first_line.strip()) > 0:
evaluates to True, because len[""] is >0.

But

for line in remaining_lines if line.strip() != ""])
removes the only entry (if line.strip() !=n ""), hence the list for the call of min() is empty!

@sphh
Copy link
Contributor

sphh commented Aug 13, 2021

I solved this by changing the statements between

and (including)
lines_adjustment = max(lines_adjustment, -max_dedent)

to

            indentations = [
                self.get_line_indentation(line)
                for line in remaining_lines if line.strip() != ""]
            if indentations:
                max_dedent = min(indentations)
                lines_adjustment = max(lines_adjustment, -max_dedent)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants