-
Notifications
You must be signed in to change notification settings - Fork 149
Implement undo and redo when removing attachments by code. #572
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
Implement undo and redo when removing attachments by code. #572
Conversation
diegoreymendez
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 few comments added.
| } | ||
|
|
||
|
|
||
| /// Return the range of an attachment with the specified identifier if any |
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.
Consistency: Let's make sure this comment is formatted with similar spacing as the rest of the comments.
Aztec/Classes/TextKit/TextView.swift
Outdated
| } | ||
|
|
||
| /// Removes the attachments that match the attachament identifier provided from the storage | ||
| /// Removes the attachment that match the attachment identifier provided from the storage |
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.
Replace "match" with "matches"
| guard let range = storage.rangeFor(attachmentID: attachmentID) else { | ||
| return | ||
| } | ||
| let originalText = storage.attributedSubstring(from: range) |
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.
From line 983 to line 990 we're repeating code that's in several other places in TextView.swift
Why not implement in TextView.swift method
func replaceCharacters(in: range, with attrString: NSAttributedString)
to take care of unified undo support?
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.
Good suggestion may I do this in another PR just dedicated to that refactor?
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.
Sure.
| /// - Parameter attachmentID: the unique id of the attachment | ||
| /// | ||
| open func remove(attachmentID: String) { | ||
| storage.remove(attachmentID: attachmentID) |
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 method is no longer called from anywhere in the code. We should remove it from TextStorage.swift.
diegoreymendez
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.
Approved.
# Conflicts: # AztecTests/TextViewTests.swift
This PR implements the capability of undo redo when removing an attachment by pressing on it and select the option remove.
To test: