Closed GoogleCodeExporter closed 9 years ago
Applied with some changes, there is also more work to be done please read below.
First thanks for this work, it is a great improvement to what was there and was
probably a bit of a trial to do.
I) Here where the changes I made:
1) AddLine is to only add a line to the document, it breaks its behavior to
have it
do auto-indent as well. I reverted this the last time for this reason. If you
need
AddLine to do an auto indent please do it another way so that it does not break
the
default behavior of this method. Arguably AutoIndent should not do the Newline
in it,
so a patch to change this behavior (without breaking the auto indenters) would
be
welcomed.
2) In the new Block/LineCaret methods, since you say the SendMsg commands don't
work
right now I commented them out, no sense in calling them if they don't work.
Also
instead of using a hardcoded message number, what is the proper
(wx.stc.STC_CMD_*)
value that it represents?
3) I thought I had mentioned to not add any vim specific code to the stc class,
especially the base class. So I moved VimMark to ed_vim.py
4) The CommandBar, FindBar methods in the base stc class should not be there.
They
should also be renamed to Show* as that is what they are doing. The buffer
classes
should not be dependent on the containers above them. For now I moved them to
the
EditraStc class and renamed them. I would like to remove them from there as
well, it
might be better to add a message or to use wx.GetApp().GetActiveWindow in the
vim
classes to get access to the active main window instead.
5) overriding built-in functions by using them as variable names, i.e) 'map' is
a
built-in function and was used as a variable.
6) Many undocumented parameters in function docstrings
II) Things that need to be done:
1) Please see TODO:CJP comments in ed_vim for things that should be looked into
further.
There are some other vim actions that are broken by this change that need to be
fixed:
1) To repeat a command the i.e) '5dd' still works but it is also valid to do
this
'd5d' to repeat a command. I personally always did it the second way when I
used to
always use vim.
2) Yank 'yy' causes the caret to jump to the beginning of the line.
3) Jump to column '|' is broken
There may be more but I didn't have time to test more thoroughly.
III) Please go through http://editra.org/vi_emulation and check how accurate it
still
is and provide updated documentation to what is supported and what is not. This
is
very important to do sooner than later.
Thanks,
Cody
Original comment by CodyPrec...@gmail.com
on 17 May 2009 at 4:13
That's cool, thanks for applying it.
Note: attached new patch file to fix some issues.
Regarding your points:
1) Is it just the AddLine method, or are you opposed to having o and O do
auto-indentation after opening a line?
2) The constants for block caret are not present in the current version of
wxPython
(atleast I couldn't find them). Scintilla defines the following constants:
#define CARETSTYLE_INVISIBLE 0
#define CARETSTYLE_LINE 1
#define CARETSTYLE_BLOCK 2
#define SCI_SETCARETSTYLE 2512
#define SCI_GETCARETSTYLE 2513
Going through the source code of wxWidgets, I found that all methods are
implemented
using SendMsg with raw numbers instead of constants! It sounds crazy but that's
what
they did, so I kinda did the same.
3) Regarding VimMark in BaseStc, well, despite its name, this method doesn't
have
much to do with vim emulation. It's just a simple method to add a bookmark and
return
its handle. The reason I put it in EditraBaseStc is that the MarkerAdd method
takes
an extra parameter MARGIN_ADD and I felt that the vim emulator doesn't need to
know
about this detail.
I didn't find a method to add a bookmark, so that method was intended to handle
that.
I admit I put an extra bit of vim stuff in there.
Would it do to rename it to AddBookmark and make it:
def AddBookmark(self):
"""Add a bookmark and return its handle"""
cline = self.GetCurrentLine()
return self.MarkerAdd(cline, MARK_MARGIN)
4) I'm not sure what the best solution would be. Again, the reason I put it
there is
because it contained a bit too much details the vim module shouldn't know about.
5) Renamed map it to cmd_map
- I wasn't aware of d5d being equivalent to 5dd, added support for it.
- Added back the | column motion.
- Added back the J join command.
- Fixed yy putting caret at start of line
- Added support for operations to operate on whole lines with certain motions
(such
as ' (goto bookmark like) and G (jump to line), as well as { and } paragraph
motions.
For instance, d'a will delete all lines from current one to the line with
bookmark
with label a, even if you're in the middle of the current line and the bookmark
labeled a is also in the middle of the other line.
- I added my own _EnsureCaretVisible because the default one in the stc acts a
bit
strange. I don't remember exactly what, but something like always scrolling the
current line to the very top, maybe even if it's visible, or something like
that.
- Added documentation for vim parser functions.
- Regarding the docs on http://editra.org/vi_emulation
Well, you don't need the distinction between one-key commands or two-key commands.
You just have to distinguish between motion commands and non-motions commands,
and
mention that c/y/d/</> operate on motion commands. Do you want me to try to
re-write
it? It's also missing the find motions and the bookmark command and motions.
- In the Clipboard class, you changed the decorator for SystemSet to
staticmethod,
why? Even though you didn't change SystemGet or any of the other ones. I don't
know,
but I changed it back to a class method. My reason for making them class
methods is
that you can call one method from inside another one using just the `cls`
parameter,
so even if the class name changes, you don't have to change its internals.
Original comment by hasan.aljudy
on 17 May 2009 at 9:35
Attachments:
Applied, looks good.
Wish this could thread messages... Replies itemized below, in reverse order
from above.
- Clipboard Class:
Its not a big deal but I simply changed it to static because its not using the
'cls'
parameter so it doesn't need to be a classmethod.
- Docs:
If the need to be rewrote, then yes. It would be good to have an itemized list
of
what commands are available and what are not.
- EnsureCaretVisible: Yea I think the stc one works under the current visibility
policy. This policy can be toggled between being strict or how many lines of
'slop'
are allowed.
#5) confirmed as fixed
#4) yea, it can just stay as it is now I will do something about it later
#3) Sounds good that works
#2) The stc is a proxy class for the Scintilla widget. Scintilla uses a message
system to allow different platforms to implement the ui. The majority of the
stc code
is autogenerated from the Scintilla interface file. Does this exist on the 2.8
branch
or only in the trunk. The scintilla in the 2.8 branch is quite old, I updated
the one
for the 2.9 branch so when thats out allot of the newer stuff will be available.
#1) I am just opposed to having it in the AddLine method. That method should
just add
a line and nothing else. If you want to do the indented version in the vim code
or
add another method like 'AddIntentedLine' that would also be ok.
Leaving open until documentation is updated.
Thanks,
Cody
Original comment by CodyPrec...@gmail.com
on 18 May 2009 at 12:26
Looks like wxwidgets svn is down right now. I will try to commit again later.
Original comment by CodyPrec...@gmail.com
on 18 May 2009 at 1:05
svn backup and commited
Original comment by CodyPrec...@gmail.com
on 18 May 2009 at 4:51
Alright,
This is related but not directly, I wanted to add an option to start in normal
mode
by default, (included in patch below), can you look at it?
Patch also puts back auto indentation to o and O commands, as well as some other
subtle changes.
Original comment by hasan.aljudy
on 19 May 2009 at 12:22
Attachments:
Applied, thanks
Original comment by CodyPrec...@gmail.com
on 19 May 2009 at 12:45
Thanks Cody,
Here's a first draft of documentation for the Vi Emulation. It's in plain text,
not a
specific markup or anything, not even reStructured or MarkDown.
Please have a look and tell me what you think.
Original comment by hasan.aljudy
on 19 May 2009 at 5:31
Attachments:
Still need to do some work on the markup but applied to website.
Any more pending updates on this or can this be considered closed for now.
Thanks,
Cody
Original comment by CodyPrec...@gmail.com
on 19 May 2009 at 11:12
I suppose it can be considered closed for now.
There's one issue though, and that is the use of [] as motions on word parts! I
kinda
decided it on a whim, it's probably not a very smart decision though; it's too
deviant from vi(m).
Original comment by hasan.aljudy
on 20 May 2009 at 1:41
Ok,
Closing this one. Please open a new ticket for any future changes.
Thanks,
Cody
Original comment by CodyPrec...@gmail.com
on 20 May 2009 at 2:12
Original issue reported on code.google.com by
hasan.aljudy
on 16 May 2009 at 7:27Attachments: