Skip to content

Conversation

@onesounds
Copy link
Contributor

@onesounds onesounds commented May 15, 2024

What's the PR

Todo

  • Don't forget Strings

Test Cases

  • If you close the window and start the cycle again, you should see the most recent query first.
  • The cycle stops when it acts on the earliest query.

ETC

  • Should the query cycle completely? My feeling is that the behavior of just stopping at the end is correct. (Current state)
    • I've tried different CLI. it should.

- Add Reset CycleIndex when close window
@onesounds onesounds marked this pull request as ready for review May 16, 2024 06:17
@onesounds onesounds added the enhancement New feature or request label May 16, 2024
@VictoriousRaptor VictoriousRaptor force-pushed the 240516GarulfQueryCycle branch from e63c42e to 3d6d79b Compare May 19, 2024 15:29

public async void Hide()
{
lastHistoryIndex = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this reset needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you close the window and start the cycle again, you should see the most recent query first.

If a user set last query style to preserve or selected that means he wants to continue the working state if so. Maybe better to reset it only when empty?

Copy link
Contributor Author

@onesounds onesounds May 20, 2024

Choose a reason for hiding this comment

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

Why is this reset needed?

When the user closes the window, the index is initialized. Lastemptyquery is just a setting about query input, so it shouldn't have anything to do with this setting. It's kind of weird when I try it in without doing this initialization. When a new window opens, we should assume that the user is starting fresh. (For example, if the user puts in a few queries and then opens the flow again an hour later and does not initialize when using this feature, the user will skip commands that he does not remember.) Get rid of it and try it. It's weird.

Copy link
Contributor Author

@onesounds onesounds May 20, 2024

Choose a reason for hiding this comment

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

If you close the window and start the cycle again, you should see the most recent query first.

If a user set last query style to preserve or selected that means he wants to continue the working state if so. Maybe better to reset it only when empty?

It is also not a good idea to initialize when user delete a query. Not closing the window means that the user intends to keep it that way. When using the function in other cli programs, the index is maintained even if the query is deleted. This is a feature that cli users will mainly use, so it's right to follow this UX.

@VictoriousRaptor
Copy link
Contributor

LGTM.

@jjw24 jjw24 added this to the 1.19.0 milestone May 22, 2024
@jjw24 jjw24 enabled auto-merge May 22, 2024 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants