Skip to content

Conversation

@marcofranzen99
Copy link
Contributor

@marcofranzen99 marcofranzen99 commented Mar 23, 2023

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 Bug: IsExecutable for Set as Background action is broken #11810

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. Rotate & Select As Background Image are hidden if no item is selected
    2. Rotate & Select As Background Image are hidden if a non image file is selected
    3. Rotate & Select As Background Image are visible if an image is selected
    4. Set as desktop slideshow is visible if multiple images are selected

@marcofranzen99
Copy link
Contributor Author

It might be better if the ViewModel would update the state itself so you don't have to call the method CheckAllFileExtensions in all three Actions but I couldn't find an easy way to implement this

@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Mar 23, 2023
@yaira2 yaira2 requested a review from QuaintMako March 23, 2023 01:10
@yaira2
Copy link
Member

yaira2 commented Mar 23, 2023

It might be better if the ViewModel would update the state itself so you don't have to call the method CheckAllFileExtensions in all three Actions but I couldn't find an easy way to implement this

Once we're finished with implementing the actions, we can look into improving the way they access the context as well as the code it uses to execute the logic.

@ferrariofilippo
Copy link
Contributor

It might be better if the ViewModel would update the state itself so you don't have to call the method CheckAllFileExtensions in all three Actions but I couldn't find an easy way to implement this

What about an abstract parent BaseSetAsAction so that we can share the code?

@marcofranzen99
Copy link
Contributor Author

What about an abstract parent BaseSetAsAction so that we can share the code?

This does not change the fact that the method CheckAllFileExtensions is called in each action. But it is a good idea to avoid the duplicate code. I will create the abstract class later

@ferrariofilippo
Copy link
Contributor

This does not change the fact that the method CheckAllFileExtensions is called in each action. But it is a good idea to avoid the duplicate code. I will create the abstract class later

Saving the result in a static property?

@marcofranzen99
Copy link
Contributor Author

Saving the result in a static property?

I am not sure if that is possible, at least I couldn't get something like that to work

# Conflicts:
#	src/Files.App/Actions/Content/ImageEdition/RotateLeftAction.cs
#	src/Files.App/Actions/Content/ImageEdition/RotateRightAction.cs
@yaira2 yaira2 requested a review from d2dyno1 March 28, 2023 13:49
yaira2
yaira2 previously approved these changes Mar 28, 2023
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@hishitetsu
Copy link
Member

Please rename this too.

// Image Edition
RotateLeft,
RotateRight,

Copy link
Member

@yaira2 yaira2 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 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Mar 28, 2023
@yaira2
Copy link
Member

yaira2 commented Mar 29, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yaira2 yaira2 merged commit 7b76c45 into files-community:main Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: IsExecutable for Set as Background action is broken

5 participants