Lots of new goodies from refactoring branch #127
Conversation
- Added PromptEditor class for promts - Began refactoring Viewer into smaller modules
- Implemented tab indicators - Improved running modules from key bindings - Added promt for creating missing dirs during save_as
| @@ -182,14 +190,14 @@ def main_loop(self): | |||
|
|
|||
| if event: | |||
| got_input = True | |||
| self.on_input(event) | |||
| self.on_input(event) # Up to 30% processing time | |||
There was a problem hiding this comment.
This comment doesn't make much sense. What's the context?
There was a problem hiding this comment.
Oops, didn't clean that up yet. I've been doing some performance analysis and finding out where the most time is consumed. The 30% indicates what proportion of the main loops execution time is spent on on_input.
There was a problem hiding this comment.
Ah, k. It might be wise to add a PERF: prefix to the comment so it can easily be found via git grep during cleanup
| "home": self.home, # Home | ||
| "end": self.end, # End | ||
| "find": self.find_query # Ctrl + F | ||
| } |
There was a problem hiding this comment.
This mapping seems pretty redundant. It's good to have some level of whitelisting to prevent exploits but here are some other options:
- A decorator which flags a function as an operation so it can be added to this list on
__init__ - A class inside of
BaseViewerwhich has the operations as methods and receivesBaseVieweras a parameter on__init__(so its calls would beself.base_viewer.cursors)
There was a problem hiding this comment.
I feel that the decorator might be a good choice for now although I'm ok with the current solution too. However in some cases the operation name might be different from the method name (like find). The current mapping is used also for the Ctrl+E run command prompt. I think it's handy to have the operation names stored as strings.
How would you incorporate the decorator into the class? I haven't used decorators much. This is related but didn't really solve my problem (specifically adding operations on __init__) http://stackoverflow.com/questions/1263451/python-decorators-in-classes ?
There was a problem hiding this comment.
The purpose of the abstraction would be to reduce the maintenance and make the functions/commands consistent (e.g. find -> find, no renaming). Otherwise, every time you add a new operation, you will be adding a new key. This isn't a lot to start but it's overwhelming when there's 50 keys.
Upon further consideration, I think an Operation class is probably easier to manage since it can be placed in a separate file.
That being said, the decorator implementation would look like (I realized that saving to a dict is much saner than marking/searching functions):
operations = {}
def operation(fn):
"""Save our function as an operation"""
operations[fn.__name__] = fn
class Viewer(object):
def run_operation(self, operation):
# Simplified for example
return operations[operation](self)
@operation
def arrow_right(self):
passFor renaming, we would define a proxy operation:
operations = {}
def operation(name):
"""Save our function as an operation"""
def operation_proxy(fn):
operations[name] = fn
return operation_proxy
class Viewer(object):
def run_operation(self, operation):
# Simplified for example
return operations[operation](self)
@operation("arrow_right")
def arrow_right(self):
passThere was a problem hiding this comment.
But for an operations class, it's much cleaner/saner:
class Operations(object):
# Mark non-operations
forbidden_operations = ('__init__', '__contains__')
def __init__(self, viewer):
# Save viewer or whatever relevant info we want (e.g. window)
self.viewer = viewer
def __contains__(self, key):
# Verify the operation exists
return hasattr(self, key) and key not in self.forbidden_operations
def arrow_right(self):
pass
def Viewer(object):
def __init__(self):
# Create an operations set for this viewer
self.operations = Operations(self)
def run_operation(self):
if operation in self.operations:
self.operations.arrow_right()There was a problem hiding this comment.
This does seem more robust so I'll transition to the Operations class when I have time. Thanks for the good examples to get started with! One question though, how does run_operation know which method to call?
There was a problem hiding this comment.
Ah, sorry. I missed a parameter for run_operation in the example. It would receive an operation parameter as it does now in the code:
Lines 637 to 646 in 819fe6e
def run_operation(self, operation):
# Simplified for example
if operation in self.operations:
self.operations.arrow_right()There was a problem hiding this comment.
Oh, I see what you were asking. It was with respect to arrow_right. We can use getattr for that. It will retrieve the instance bound function for us to run:
def run_operation(self, operation):
# Simplified for example
if operation in self.operations:
operation_fn = getattr(self.operations, operation)
operation_fn()
There was a problem hiding this comment.
Oh yeah, ofcourse we have getattr. Looks nice and straightforward, and I like the idea of a separate file for operations. A third option I've been considering is moving all operations to dedicated modules, any thoughts on that? I'm not quite sure if its a good idea or worth it.
For now though the Operations class feels like the way to go. I'll shedule it for my next round of refactoring.
There was a problem hiding this comment.
I think breaking out operations to their own files is reasonable. However, it's mostly useful when the contents of an operation are overwhelming for a developer. I think they are pretty bite-sized for now so a class works.
My preferred approach for something like that is usually a hybrid; a core class which has lots of small/simple operations with imported external functions for more complex ones:
from suplemon.operations.super_complex import super_complex
class Operations(object):
def arrow_right:
# ...
@classmethod
def add_operation(cls, name, fn):
"""Helper to attach a new operation to our operations class""
cls.name = fn
# Add external operations
Operations.add_operation('super_complex', super_complex)| """Move cursors right.""" | ||
| for cursor in self.cursors: | ||
| line = self.lines[cursor.y] | ||
| if cursor.y != len(self.lines)-1 and (cursor.x >= len(line) or len(line) == 0): |
There was a problem hiding this comment.
While the logic will work, shouldn't we check EOL before checking the next line exists? (e.g. if we are going to scroll to the next line, then verify there is a next line)
There was a problem hiding this comment.
In fact, we can prob move the cursor.y check inside of this if block since we don't want an else scenario. Then, we can prob convert the elif to an else
There was a problem hiding this comment.
line = self.lines[cursor.y]
# If we are at the end of the line
if cursor.x >= len(line) or len(line) == 0:
# If there is another line, then move down
if cursor.y != len(self.lines)-1
cursor.move_down()
cursor.set_x(0)
# Otherwise, move the cursor right
else:
cursor.move_right()There was a problem hiding this comment.
Thanks, that does look better. These were implemented long ago when I just needed to get things working, and didn't pay as much attention to code style as I should have.
| cursor.move_up() | ||
| cursor.set_x(len(self.lines[cursor.y])+1) | ||
| self.move_cursors((-1, 0)) | ||
| self.scroll_up() |
There was a problem hiding this comment.
Thinking about cursors more... I think move_cursors is a good start but maybe rename it to move_cursors_relative and let that compute absolute positions. Then define a move_cursors_absolute.
move_cursors_absolute will be practical for performance so kudos on that.
For high level user interfaces though, we will want to handle lots of different cases (e.g. moving by 2 characters, 3 characters, 1 word, 1 line). So it's something to think about (e.g. reconsider arrow_left/arrow_right as more move_by_character(forward=False)/move_by_character(forward=True))
There was a problem hiding this comment.
I must admit that the sublime way looks very nice and that's how I'd like it to work in suplemon too. This requres some more refactoring in many places so I'll postpone this for later. I'll try to finalize and 
the bug fixes and new stuff in this PR first.
Thanks for the suggestion, I'll definitely come back to this when I have time.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
I'll merge now so I can release the new input prompt and other improvements. I'll continue with more refactoring later. |
Lots of new goodies from refactoring branch
I somehow went into full rampage mode and began major refactoring.
Current changes:
format).