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

Custom undo/redo implementation (#97) #98

Merged
merged 6 commits into from
Oct 31, 2022
Merged

Conversation

alexeyinkin
Copy link
Contributor

@alexeyinkin alexeyinkin requested a review from Malarg October 28, 2022 14:29
@alexeyinkin alexeyinkin marked this pull request as ready for review October 28, 2022 14:29
Timer? _debounceTimer;

@visibleForTesting
final stack = <CodeHistoryRecord>[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it is not a stack. Please name it suitably, or create data structure, which will able to push and popRange. It may be named LimitStack with max records at constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no problem calling a variable after our usage intention and not after a class. If we want to push and pop with a list, we can call it 'stack'.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think that this approach makes ambiguily while reading the code. If we rename this field to recordList and methods like saveRecord, removeRecords and so on, code will be core clear to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+


void beforeChanged(Code code, TextSelection selection) {
_dropRedoIfNeed();
bool save = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please name it using convention of naming bool variables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The appropriate mode is 'should' which would turn a 4-char variable to a 10-char variable, so I would rather not.
Naming is less strict with local variables than it is with wider-scoped ones.

Copy link
Collaborator

@Malarg Malarg Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is less strict with local variables than it is with wider-scoped ones.

Considering broken windows theory it isn't. I believe, that code have to be clean, regardless it placement. Specifically at this place renaming won't create any additional line breaks, but code will be more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+

lib/src/history/code_history_controller.dart Outdated Show resolved Hide resolved
lib/src/code_field/text_editing_value.dart Outdated Show resolved Hide resolved
@alexeyinkin alexeyinkin requested a review from Malarg October 31, 2022 08:31
@alexeyinkin alexeyinkin merged commit c64d071 into main Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom undo/redo implementation
2 participants