Skip to content

Code Quality: Only execute actions once #13397

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 13 commits into from
Closed

Code Quality: Only execute actions once #13397

wants to merge 13 commits into from

Conversation

jvestell
Copy link
Contributor

@jvestell jvestell commented Sep 19, 2023 •
edited
Loading

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Only execute certain actions once #11737

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Open app ...
    2. hold ctrl + t and see that tabs don't continuously open
    3. press ctrl + t a few times
    4. hold ctrl +w and see that tabs don't continuously close
    5. press ctrl + w a few times
    6. refresh the same

Screenshots (optional)
Add screenshots here.

@jvestell
Copy link
Contributor Author

@yaira2 Ready for review. Fully implemented.

@yaira2 yaira2 changed the title Feature: Only Execute Certain Actions Once, Closes#11737 Feature: Only execute actions once Sep 20, 2023
@yaira2 yaira2 self-requested a review September 21, 2023 20:25
@yaira2 yaira2 changed the title Feature: Only execute actions once Code Quality: Only execute actions once Sep 21, 2023
@jvestell
Copy link
Contributor Author

@yaira2 Thank you for the review. I'm waiting for further instruction. Is this on track to be merged to main?

@0x5bfa
Copy link
Member

0x5bfa commented Oct 2, 2023

redo and undo actions shouldn’t be limited, but otherwise I think all actions should be.

@yaira2
Copy link
Member

yaira2 commented Oct 2, 2023

redo and undo actions shouldn’t be limited

I think they should, we wouldn't want someone undoing multiple operations by mistake.

@ferrariofilippo
Copy link
Contributor

would it be worth extending this behavior to every action? If we do this, we can handle it across the board instead of duplicating the code for each action.

I think we should do that, I can't think of any use-case where a user needs to execute the same action over and over. It may also prevent some bugs we are not aware of.

@jvestell
Copy link
Contributor Author

@yaira2 @ferrariofilippo change fully implemented for all actions. Ready for you.

QuaintMako
QuaintMako reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

A few feedbacks, mostly on the format.
The functionnality looks good to me, I cannot test it for now.

@jvestell
Copy link
Contributor Author

@QuaintMako @yaira2 Suggestions taken and implemented. I left one action with an optional value of 1000 milliseconds for demonstration. The others are defaulted at 800ms.

0x5bfa
0x5bfa reviewed Oct 16, 2023
View reviewed changes
Copy link
Member

@0x5bfa 0x5bfa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

being particular but needed. LGTM otherwise, thank you!

PS.

Can you fix merge conflicts? You can fix most of all by fetching from upstream/main; otherwise you can fix in merge editor in VS2022.

src/Files.App/Actions/DebounceAction.cs Outdated
{
public abstract class DebouncedAction : IAction
{
private DateTime lastExecuted = DateTime.MinValue;
Copy link
Member

Choose a reason for hiding this comment

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

lastExecutedTime

src/Files.App/Actions/DebounceAction.cs Outdated
{
private DateTime lastExecuted = DateTime.MinValue;

private readonly TimeSpan debounceTime;
Copy link
Member

Choose a reason for hiding this comment

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

debounceTimeSpan

@jvestell jvestell force-pushed the 11737_Execute_Limits_Jerrod branch from 8f185f3 to 8139d77 Compare October 17, 2023 03:51
@jvestell
Copy link
Contributor Author

jvestell commented Oct 17, 2023 •
edited
Loading

@0x5bfa @yaira2 @QuaintMako

Branch synced with main as of now. No conflicts. Ready to merge. The constructor suggestion you gave me @QuaintMako to eliminate nullability gave errors that I'd need to look more into later. I think this is good for now.

@jvestell
Copy link
Contributor Author

@yaira2 Could I get an update on this? I think it's ready to merge. Thank you.

@yaira2 yaira2 requested review from QuaintMako and 0x5bfa October 20, 2023 19:41
QuaintMako
QuaintMako approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

LGTM.

0x5bfa
0x5bfa approved these changes Oct 21, 2023
View reviewed changes
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2
Copy link
Member

yaira2 commented Oct 22, 2023

@HorseBandit can you take a look at the build error?

@jvestell
Copy link
Contributor Author

@yaira2 Fresh sync followed by a merge of 'main' into this branch. There are no build errors.

@hecksmosis
Copy link
Contributor

I think this is a pipeline bug iirc, don't remember what we used to do to fix it

@yaira2
Copy link
Member

yaira2 commented Oct 22, 2023

@HorseBandit do the test projects run locally? It seems to be failing in the PR.

@jvestell
Copy link
Contributor Author

jvestell commented Oct 22, 2023 •
edited
Loading

@yaira2 Yes they do. Test results for main and this branch are identical.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yaira2
Copy link
Member

yaira2 commented Oct 24, 2023

@HorseBandit I think the best option is to duplicate the branch and create a new PR.

@yaira2 yaira2 mentioned this pull request Oct 27, 2023
Closed
8 tasks
@yaira2 yaira2 force-pushed the main branch 5 times, most recently from a827a37 to 20e04d0 Compare November 2, 2023 18:32
@yaira2 yaira2 closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x5bfa 0x5bfa 0x5bfa approved these changes

@QuaintMako QuaintMako QuaintMako approved these changes

@yaira2 yaira2 Awaiting requested review from yaira2

Assignees
No one assigned
Labels
needs - code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Only execute certain actions once
6 participants
@jvestell @0x5bfa @yaira2 @ferrariofilippo @hecksmosis @QuaintMako