-
Notifications
You must be signed in to change notification settings - Fork 4
Added automatic tests and fixes for all of those tests #8
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
base: master
Are you sure you want to change the base?
Conversation
|
I have implemented “Paste from History” feature (as I decided little earlier). |
…xt on Windows (bug was observed while pasting multi-line text [copied from Sublime Text] into Windows Notepad)
Traceback (most recent call last):
File "...\SublimeText\sublime_plugin.py", line 818, in run_
return self.run(edit)
File "...\SublimeText\Data\Packages\CopyEdit\copy_edit.py", line 68, in run
if pasteboard != '\n'.join([s[0] for s in selection_strings]):
TypeError: sequence item 0: expected str instance, NoneType found
…ing selected text by pressing Delete or Backspace
This restores the original sublime text behaviour of copy and
pasting without selection.
Example: Caret=|
Copy Paste
aaaa aaaa
bb|bb bbbb
cccc bb|bb
cccc
Without this patch the carets return on paste to the front of the line
…e, so "append" is used instead
adzenith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A long-overdue review! I appreciate the contributions!
| @@ -0,0 +1,3 @@ | |||
| [ | |||
| { "keys": ["ctrl+shift+v"], "command": "paste_from_history_edit" } | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we should bind keys by default - what do you think? Maybe we should let people bind their own keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can always rebind keys how they like, but I consider that this [paste from history] operation is more needed than paste_and_indent which is assigned by default in SublimeText at ctrl+shift+v.
Besides paste_and_indent is somewhat broken and can not be reliably used, for example:
If you have code like this:
if nesting_level == 0:
break
and put cursor after last line with break, then command past_and_indent align new_code wrong:
if nesting_level == 0:
break
new_code
Another example:
If you have
if nesting_level == 0: # comment
break
then cut line with break:
if nesting_level == 0: # comment
and immediately execute past_and_indent command:
if nesting_level == 0: # comment
break
So, I would rather go with regular paste command and realign all code manually.
|
|
||
| str_index = 0 | ||
| new_sels = [] | ||
| for sel in self.view.sel(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it was just for sel in reversed(self.view.sel()):? Would that fix it? That way you're replacing from the end. Or maybe I'm misunderstanding the bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify which commit your question is related to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've found the related commit, but I can not think of how it can be reimplemented (simple adding reversed is not enough because of str_index usage inside that loop).
You can play with the code yourself if you wish, but do not forget to run tests (your suggestion breaks test (1)).
| def on_text_command(self, view, command_name, args): | ||
| if command_name in ["cut", "copy", "paste", "paste_from_history"]: # actually adding "paste_from_history" here does not make any sense because this command is disabled after startup of SublimeText | ||
| return (command_name + "_edit", args) | ||
| if command_name in ["left_delete", "right_delete"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will trigger for every character you delete, won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is.
I can't remember why I've done this this way, but as I can see performance impact is negligible, so I do not see the problems with this approach.
| def skip_comments(): | ||
| nonlocal pos | ||
| while tests[pos] == '[': | ||
| while tests[pos] == '[': # [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these comments for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To balance opening and closing brackets.
| new_sel_strings.append((self.view.substr(s), False)) | ||
| elif copy_with_empty_sel: | ||
| new_sel_strings.append((self.view.substr(self.view.full_line(s)), True)) | ||
| new_sel_strings.append((self.view.substr(self.view.line(s)) + "\n", True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What bug does this fix? Why would you want to manually add a newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fixes case when selection is empty and when caret is placed at the last line of file and when there is no newline at end of file. Do you use copy with empty selection? If not, then you wouldn't understand this fix.
copy_edit.py
Outdated
| return | ||
|
|
||
| if numsels == 1: # To fix TN 7 | ||
| selection_strings = [(("" if selection_strings[0][1] else "\n") # проверка нужна, так как если selection_strings[0][1] == True, то \n уже есть в конце строки |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could translate this? I'm not positive what this is fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have added commit with translation of this comment.
About "what this is fixing" please see test, which was added in this commit (if you wish I can explain what it means, but it's obvious [DA >‘1’<2 means insert text 12 and select 1]).
| for command in commands: | ||
| cmd = re.sub(R' \[[^]]+]', '', command[1]) # remove comments | ||
| buffer.run_command("append", { "characters": "".join([c[0] + '. ' + c[1] + "\n" for c in commands]) } ) # || "insert" is not working totally correctly here, so "append" is used instead | ||
| # \\ To see what is the difference try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put these comments on their own lines instead of at the end of other lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't totally understand you (do you mean place all 5 comment lines before buffer.run_command("append", ... or something else), but you can move this comments how you like. [Personally I see no problem with this comments because wide monitors is so widespread nowadays.]
| string = next(sel_strings) | ||
| for sel in self.view.sel(): | ||
| replace_region = sublime.Region(self.view.line(sel.begin()).begin()) if string[1] else sel | ||
| replace_region = sublime.Region(self.view.line(sel.begin()).begin()) if string[1] and sel.size() == 0 else sel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, it fixes test TN 8, as commit message says. :-)
TN 8
DA‘1>‘’<
2’
CO copy
DA‘>‘1
’<2’
CO paste
CR‘1
>‘’<2’
If you still don't get it [i.e. how to read this tests' notation], here is the explanation:
DA‘1>‘’<
2’
means initialize DAta (or DAno in Russian) with:
1|
2
(here | means cursor/caret)
CO copy means run COmmand copy (like view.run_command("copy")).
DA‘>‘1
’<2’
means replace DAta with:
|>1
<|2
(here |> means selection start, and <| means selection end)
CO paste means run COmmand paste (like view.run_command("paste")).
And at last
CR‘1
>‘’<2’
means that Correct Result is should be the following:
1
|2
Before this commit the result was this:
1
1
|2
…nd remove one wrong/strange[/unnecessary] comment
No description provided.