Skip to content

feat: add date_format and hour_format settings #898

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

Closed
wants to merge 3 commits into from

Conversation

JCC1998
Copy link
Contributor

@JCC1998 JCC1998 commented Mar 30, 2025

Summary

Added date_format and hour_format settings to allow customization on how to visualize dates and hours in the preview_panel
Settings panel with en-En locale format and 24h display:
image
image

Spanish date format and 24h:
image
image

Spanish format and 12h:
image
image

International format:
image
image

Tasks Completed

  • Platforms Tested:
    • Windows x86
    • Windows ARM
    • macOS x86
    • macOS ARM
    • Linux x86
    • Linux ARM
  • Tested For:
    • Basic functionality
    • PyInstaller executable

@CyanVoxel CyanVoxel added Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience Type: Translations Modifies translation keys or translation capabilities. Status: Review Needed A review of this is needed labels Mar 30, 2025
@CyanVoxel CyanVoxel moved this to 🏓 Ready for Review in TagStudio Development Mar 30, 2025
@CyanVoxel CyanVoxel added this to the Alpha v9.5.3 milestone Mar 30, 2025
@CyanVoxel CyanVoxel linked an issue Mar 31, 2025 that may be closed by this pull request
3 tasks
@CyanVoxel CyanVoxel moved this from 🏓 Ready for Review to 👀 In review in TagStudio Development Mar 31, 2025
Copy link
Member

@CyanVoxel CyanVoxel left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! I've caught up on all the discussion in the Discord and on #873.

First off, I think the 24-hour time as a boolean setting is definitely the right move. A couple notes on it though - I see that the checkbox label for this is hard-coded as "24H" when this should be a translated key. Actually, instead of having an "Hour Format" label with a checkbox that's next to another label, I'd recommend renaming "Hour Format" to "24-Hour Time" and leaving the checkbox on its own since there will never be any other options than on or off. This also matches how the settings work in macOS, for example.
image
I also noticed you were having some difficulties with selectively showing AM or PM in the Discord, I apologize for not getting back to the question sooner. You have a method that returns either "%H:%M:%S" for 24-hour time or "%I:%M:%S" for 12-hour which is fine, but then you have some manual logic for adding on "AM" or "PM" when you should be able to use the "%p" inside the "%I:%M:%S %p" string. It seems that this wasn't working for you due to it conflicting with the locale setting, but I'm not sure if that's because you were manually setting the locale or if this was an issue with the default Python locale on your machine (more on locale issues below).

For the date formats, I'm noticing a few issues with the approach of setting the program's locale as well as the given "English" and "International" options. Besides the issues you may have experienced with "%p" not working as intended, different systems will handle locales differently. For example, "en-EN" does not seem to be a "real" locale and throws an error under macOS:

2025-03-31 10:18:36 [error    ] [Preview Panel] Error updating selection error=Error('unsupported locale setting')
Traceback (most recent call last):
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/src/tagstudio/qt/widgets/preview_panel.py", line 154, in update_widgets
    self.file_attrs.set_locale()
  File "/Users/cyanvoxel/Files/GitHub/TagStudio/src/tagstudio/qt/widgets/preview/file_attributes.py", line 260, in set_locale
    locale.setlocale(locale.LC_ALL, "en-EN")
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/locale.py", line 615, in setlocale
    return _setlocale(category, locale)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
locale.Error: unsupported locale setting

In addition, setting locale.LC_ALL changes much more than just the date and time formats and behaves differently across different systems and configurations. As mentioned by Qronikarz here, American English specifically uses the "MM/DD/YYYY" format while other English-speaking countries tend to use "DD/MM/YYYY" so having "English" as the synonymous with "MM/DD/YYYY" would be incorrect. Furthermore, having "International" as an ISO 8601 "YYYY-MM-DD" option doesn't quite cover all formats, for example the "DD/MM/YYYY" format.

I think setting the program's locale for dates and times is just creating more hassle than it's worth, and it would be easier and more flexible to leave the locale (at least for date and time) as the default and to offer specific date and time formats that are separated from the user's locale. This is also how I tend to see these options presented in frontends, where users are allowed to specify formats other than their explicit system locale. For example, here's the setting on macOS:
image
And the settings on Windows 10:
win_short_date win_long_date

Personally I prefer the macOS-style options, but still with the day of the week shown (Maybe that should be an individual setting too?). The setting could probably be saved as the raw date format string inside the settings file, or use updated enums to include representations for each of these. While it would be nice to have an option to follow the system's format, I feel it would be a lot more hassle that it's worth to do that for each system, assuming it's even possible on all systems. I often have to change my time format to 24-hour in applications even though I have it set as such in macOS, for example. We also don't have a system language option and instead default to English until the user modifies it, so there's no expectation or president for a system option here to be strictly necessary at this time.

Lastly, a general note on some of the code placement - the new methods inside the file_attributes.pyshould be moved to their own file since they're not specific to the file attributes file and can be used elsewhere.

Please feel free to let me know if you have any more questions or would like some help, either here or on the Discord (I'll try to get back to you more quickly!). Thank you again for your work so far on this!

@CyanVoxel CyanVoxel added Status: Changes Requested Changes are quested to this and removed Status: Review Needed A review of this is needed labels Mar 31, 2025
@CyanVoxel CyanVoxel moved this from 👀 In review to 🚧 In progress in TagStudio Development Mar 31, 2025
@JCC1998 JCC1998 closed this Apr 1, 2025
@JCC1998 JCC1998 deleted the feature/issue-873 branch April 1, 2025 17:45
@github-project-automation github-project-automation bot moved this from 🚧 In progress to ✅ Done in TagStudio Development Apr 1, 2025
@JCC1998
Copy link
Contributor Author

JCC1998 commented Apr 1, 2025

I messed up a bit by deleting the branch. I'm so used to do it in my job that didn't expect GitHub to close the PR automatically, because Azure still mantains the PR open
The new PR: #904
And sorry again for messing up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Changes Requested Changes are quested to this Type: Enhancement New feature or request Type: Translations Modifies translation keys or translation capabilities. Type: UI/UX User interface and/or user experience
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add locale to dates
2 participants