Skip to content
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

Show description for cron expressions #839

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

micahmo
Copy link

@micahmo micahmo commented Jun 5, 2023

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • New feature or enhancement
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Issue Number: #829

What is the new behavior?

This PR introduces a human-readable description of cron expressions to help users understand what they mean (in addition to seeing the evaluated times).

DevToys-Cron-Demo.mp4

Other information

New PostBuildEvents

The CronExpressionDescriptor package comes with .resource.dll files for multiple cultures. By default, a folder containing each culture's resource will be copied to the output

image

However, UWP recompiles these into its own resources.pri file (source). I didn't want to clutter the output with the unnecessary DLLs, so I added post-build events to the project files to clean these up. I tested multiple cultures, and it is able to correctly generate cron expressions for the current culture. If this is overkill and we don't care about these folders, I can revert the project file changes.

P.S. I first tried to fix this the "right" way by explicitly referencing the undesired DLLs and setting CopyLocal to false as per this recommendation, but that didn't work.

Limitations

Cron Expression Descriptor doesn't support non-standard expressions like @daily or @weekly.

Quality check

Before creating this PR:

  • Did you follow the code style guideline as described in CONTRIBUTING.md
  • Did you build the app and test your changes?
  • Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • Did you verify that the change work in Release build configuration
  • Did you verify that all unit tests pass
  • If necessary and if possible, did you verify your changes on:
    • Windows
    • macOS (DevToys 2.0)
    • Linux (DevToys 2.0)

@veler
Copy link
Collaborator

veler commented Jun 6, 2023

Hello,

Thank you very much for your PR :)

However, UWP recompiles these into its own resources.pri file (Humanizr/Humanizer#317 (comment)). I didn't want to clutter the output with the unnecessary DLLs, so I added post-build events to the project files to clean these up. I tested multiple cultures, and it is able to correctly generate cron expressions for the current culture. If this is overkill and we don't care about these folders, I can revert the project file changes.

That's quite interesting and I admit I'm puzzled by the fact that without the culture folder, the app still localize correctly the text. Is it possible that your post build event clears the folders from bin folder but doesn't clear them from the generated .msix file?

I will double check that, if you don't mind.

@micahmo
Copy link
Author

micahmo commented Jun 6, 2023

I admit I'm puzzled by the fact that without the culture folder, the app still localize correctly the text.

UWP is very new to me so I am by no means an expert here. However, even the existing application (before the new package) does not have resource folders or DLLs, yet it is able to localize correctly. I believe it must load resources from resources.pri. You can use makepri.exe to decompile this file and see all the strings. If you remove that file from the output, the app fails to start.

it possible that your post build event clears the folders from bin folder but doesn't clear them from the generated .msix file?

Yes please verify that since I do not know how to generate the .msix. Thanks!

@veler
Copy link
Collaborator

veler commented Jun 8, 2023

I admit I'm puzzled by the fact that without the culture folder, the app still localize correctly the text.

UWP is very new to me so I am by no means an expert here. However, even the existing application (before the new package) does not have resource folders or DLLs, yet it is able to localize correctly. I believe it must load resources from resources.pri. You can use makepri.exe to decompile this file and see all the strings. If you remove that file from the output, the app fails to start.

it possible that your post build event clears the folders from bin folder but doesn't clear them from the generated .msix file?

Yes please verify that since I do not know how to generate the .msix. Thanks!

Alright, I gave it a try by creating an MSIX package in Release mode. With the current PR, the text in the field you added stays in English even if DevToys is in French. By NOT removing the language folders, the text follows the right language.

Therefore, please remove these PostBuildEvent 😉

@micahmo
Copy link
Author

micahmo commented Jun 8, 2023

Alright, I gave it a try by creating an MSIX package in Release mode. With the current PR, the text in the field you added stays in English even if DevToys is in French. By NOT removing the language folders, the text follows the right language.

Therefore, please remove these PostBuildEvent 😉

Gladly! My only concern was cluttering the output, but if these folders/dlls are necessary for localization, of course we will keep them. Thank you for testing this! Reverted in 302c1af.

@veler veler linked an issue Jun 8, 2023 that may be closed by this pull request
Copy link
Collaborator

@veler veler left a comment

Choose a reason for hiding this comment

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

PR approved! Thank you so much again for this contribution! Keep the good work. We love all your recent contributions.

@veler veler merged commit e2a46f8 into DevToys-app:main Jun 9, 2023
@micahmo micahmo deleted the feature/cron-description branch June 9, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Human-readable description of cron expression
2 participants