Skip to content

#1404 Serial Monitor message history #1467

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

#1404 Serial Monitor message history #1467

wants to merge 2 commits into from

Conversation

dwightfowler-cd
Copy link
Contributor

@dwightfowler-cd dwightfowler-cd commented Sep 19, 2022 •
edited by kittaakos
Loading

Motivation

Address PR #1453 comment

Change description

Conform to community coding guidelines

Other information

Closes #1404

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos mentioned this pull request Sep 20, 2022
Closed
4 tasks
@kittaakos kittaakos changed the title #1404 #1404 Serial Monitor message history Sep 21, 2022
@kittaakos kittaakos added type: enhancement Proposed improvement topic: code Related to content of the project itself topic: serial monitor Related to the Serial Monitor labels Sep 21, 2022
@dwightfowler-cd
Copy link
Contributor Author

dwightfowler-cd commented Sep 24, 2022 •
edited
Loading

@kittaakos, is everything good to go for this PR? Is there anything else I should do to get this ready?

@nmzaheer
Copy link
Contributor

I have reviewed the code and have some queries:

  1. What happens when I press the Up Arrow to retrieve the previous command and then press Enter? Does it get added to the list or since it is a duplicate do you drop it?
  2. The legacy code also had a provision of handling Escape key to reset? Are we dropping it? Or is it handled internally?

I will manually test your code out and give you feedback maybe tomorrow

kittaakos
kittaakos suggested changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I gave it a try, but it behaves differently than in IDE 1.x.

Steps to reproduce:

  • send one,
  • send two,
  • press up -> the history must be two, but it's one

If you fix this, I take another look.

IDE 1.x:

ide1x_history.mp4

IDE2:

history_ide2.mp4

arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
@@ -6,6 +6,74 @@ import { BoardsServiceProvider } from '../../boards/boards-service-provider';
import { MonitorModel } from '../../monitor-model';
import { Unknown } from '../../../common/nls';

class RingList {
Copy link
Contributor

Choose a reason for hiding this comment

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

RingList implies that message history is a cycle.

With the following sent messages:

  • one,
  • two, and
  • three.

I would expect to get three after one. But no, so the class name is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I debated with myself over that name. I'm changing it to HistoryStack. It's more descriptive.

arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
private index: number;
private end: number;

constructor(size: number = 100) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be constructor(size = 100) {.

arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
this.init(size);
}

private init(size: number = 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be private init(size = 100) {

Please respect the position of the curly braces.

@nmzaheer
Copy link
Contributor

It's almost been 3 weeks since the last update. How can I contribute and complete the required changes so that this feature is completed?

@nmzaheer nmzaheer mentioned this pull request Oct 16, 2022
Merged
4 tasks
@kittaakos kittaakos added the status: changes requested Changes to PR are required before merge label Oct 20, 2022
@kittaakos
Copy link
Contributor

Thank you for your initial work on this feature, @dwightfowler-cd.

There is another PR: #1562 based on your contribution. Please follow that one.

I am closing this as a duplicate.

@kittaakos kittaakos closed this Oct 21, 2022
@kittaakos kittaakos added conclusion: duplicate Has already been submitted and removed status: changes requested Changes to PR are required before merge labels Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kittaakos kittaakos kittaakos requested changes

Assignees
No one assigned
Labels
conclusion: duplicate Has already been submitted topic: code Related to content of the project itself topic: serial monitor Related to the Serial Monitor type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Serial Monitor message history
3 participants
@dwightfowler-cd @nmzaheer @kittaakos