-
-
Notifications
You must be signed in to change notification settings - Fork 491
#1404 Added Message History to Serial Monitor #1562
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
Conversation
There was an error in the next method wherein the last value was being shown twice. It has been corrected with the latest commit. I have also downloaded the artifact and tested against it.
I had started with the code in #1467 . The discussion there was about renaming the RingList to a different name as Ring implies cyclical behavior. Although HistoryStack was suggested as an alternative, I believed that was wrong. The functionality required traversal through the data structure and hence HistoryList was better name,
The expected behavior should be as in Arduino 1.x . Let me know once you have tested it.
Is the team getting close to a release?
I will ensure this feature is part of the next patch release of IDE2. We plan to roll out the next version this month. Please bear with us.
@dwightfowler-cd, if you do not plan to do follow-ups on #1467, please close it. Thanks for keeping the repo clean ❤️
@kittaakos Did you have a chance to test the behaviour? Do you have any feedback for me?
It's working great. Thank you for your contribution. I believe the logic can be simplified. Let's remove the unnecessary state from the HistoryList
. See my proposal here. What do you think?
Please give it a try. If you like it, you can cherry-pick
the changes, or you can add my fork to your remotes and pull in the changes.
Thank you for testing. Your implementation looks much neater and simpler than what I had submitted.
I am testing your implementation of HistoryList with this code. However, it skips the first entry when next
method is run. I will have to correct the code accordingly.
I'll get back to you with the changes.
Edit
I think the below code implementation is more apt since the previous method should not return an empty string when it has reached the beginning of the list. In IDE 1.x, once the index reaches the beginning of the list, the first element is returned always.
previous(): string { if (this.index === -1) { this.index = this.items.length - 1; return this.items[this.index]; } if (this.hasPrevious) { return this.items[--this.index]; } return this.items[this.index]; }
Looking forward to your feedback.
- The implementation has been taken from @kittaakos repo https://github.com/kittaakos/arduino-ide/blob/d10de017362989b62d01991a33eaef9e71a6aec4/arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx - The previous method has been modified to ensure the first element instead of an empty string is returned if the index is at the beginning of the list. - The push method has been modified to check if the current command is same as the last command. If same then, it is not added to the list else it is added.
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.
The code is looking good and the monitor history works as in IDE 1.x 👍
arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
Outdated
Show resolved
Hide resolved
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.
Thank you! It's looking good.
Uh oh!
There was an error while loading. Please reload this page.
Motivation
Feature added for message history in Serial Monitor
Change description
Utilizes the RingList implementation of @dwightfowler-cd here with some corrections for traversal of the list
Other information
Reviewer checklist