Skip to content

PTBAS-737: using consistent english language now - #184

Closed
MomdAli wants to merge 5 commits into
developfrom
bugfix/PTBAS-737_consistentLanguage
Closed

PTBAS-737: using consistent english language now#184
MomdAli wants to merge 5 commits into
developfrom
bugfix/PTBAS-737_consistentLanguage

Conversation

@MomdAli

@MomdAli MomdAli commented Sep 5, 2025

Copy link
Copy Markdown
Collaborator

Locale management and formatting improvements:

  • Added setSystemLocale, getSystemLocale, and applySystemLocaleOnDate methods to the DateFormatter utility class, allowing centralized management of the application's locale and consistent formatting of dates in DatePicker components. (DateFormatter.java) [1] [2]
  • On application startup, the system locale is now set in DateFormatter, and the default locale is explicitly set to English to ensure predictable behavior. (App.java) [1] [2]

mal-saiaf
PTBAS-737: using consistent english language now
a0ef07b 
* using system locale for date/times
@MomdAli
MomdAli requested a review from Death111 September 5, 2025 11:05
@MomdAli MomdAli self-assigned this Sep 5, 2025
@MomdAli
MomdAli marked this pull request as ready for review September 5, 2025 12:10
@MomdAli

MomdAli commented Sep 5, 2025

Copy link
Copy Markdown
Collaborator Author

@CodiumAI-Agent /review

@QodoAI-Agent

QodoAI-Agent commented Sep 5, 2025 •
edited
Loading

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit c9cc12b)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Locale Initialization Order

The app sets DateFormatter's system locale to the current default, then immediately overrides the JVM default to English. Verify that components created before/after this sequence use the intended locale and that English default does not unintentionally override user expectations elsewhere.

 
DateFormatter.setSystemLocale(Locale.getDefault());
Locale.setDefault(Locale.ENGLISH);
final DefaultExceptionHandler defaultExceptionHandler = new DefaultExceptionHandler();
Thread Safety

The static mutable field systemLocale is updated via setSystemLocale without synchronization. If accessed or changed from multiple threads (UI vs. background), consider making it volatile or using synchronized access to avoid race conditions.

 
public static void setSystemLocale(Locale locale) {
   systemLocale = locale;
}

public static Locale getSystemLocale() {
   return systemLocale;
}
Fragile UI Locale Assertions

The test checks for localized strings like "Montag" and "06.2024". This may be brittle across JDK vendors or OS settings. Consider asserting via formatter pattern or parsing round-trip rather than string contains.

 
@Test
void applySystemLocaleShouldSetDatePickerLocale() {
   // GIVEN
   Locale defaultLocale = Locale.getDefault();
   Locale.setDefault(Locale.ENGLISH);
   DateFormatter.setSystemLocale(Locale.GERMAN);
   DatePicker datePicker = new DatePicker();
   LocalDate date = LocalDate.of(2024, 6, 3);

   // WHEN
   DateFormatter.applySystemLocaleOnDate(datePicker);
   datePicker.setValue(date);

   // THEN
   String shown = datePicker.getEditor().getText();
   assertTrue(shown.contains("Montag"));
   assertTrue(shown.contains("06.2024"));

   Locale.setDefault(defaultLocale);
}

Death111
Death111 requested changes Sep 14, 2025
View reviewed changes
Comment thread src/main/java/de/doubleslash/keeptime/App.java Resolved
@MomdAli
MomdAli requested a review from Death111 September 15, 2025 08:51
Death111
Death111 requested changes Sep 16, 2025
View reviewed changes
Comment thread src/test/java/de/doubleslash/keeptime/common/DateFormatterTest.java Resolved
Comment thread src/test/java/de/doubleslash/keeptime/common/DateFormatterTest.java Resolved
Comment thread src/main/java/de/doubleslash/keeptime/common/DateFormatter.java Resolved
@Death111

Death111 commented Oct 2, 2025

Copy link
Copy Markdown
Collaborator

@MomdAli I was using it the last weeks and I did not notice issues - only annoying thing is the calendar view that sunday is the first day of the week. can this be changed / also localized somehow?
grafik

@QodoAI-Agent

QodoAI-Agent commented Oct 2, 2025

Copy link
Copy Markdown

Persistent review updated to latest commit c9cc12b

Death111
Death111 reviewed Oct 20, 2025
View reviewed changes
Comment thread src/main/java/de/doubleslash/keeptime/common/DateFormatter.java

datePicker.setOnShowing(e-> Locale.setDefault(Locale.Category.FORMAT, systemLocale));
datePicker.setOnHiding(e-> Locale.setDefault(Locale.Category.FORMAT, Locale.getDefault()));
datePicker.setOnAction(e-> Locale.setDefault(Locale.Category.FORMAT, Locale.getDefault()));

@Death111 Death111 Oct 20, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these resets did not make sense for me, as you reset it to the "wrong" locale. But it showed me this way to only set the locale for FORMAT. I created #191 to most likely replace this PR - lets review it.

@Death111 Death111 mentioned this pull request Oct 20, 2025
Merged
@Death111

Death111 commented Oct 24, 2025

Copy link
Copy Markdown
Collaborator

Implemented via #191

@Death111 Death111 closed this Oct 24, 2025
@Death111
Death111 deleted the bugfix/PTBAS-737_consistentLanguage branch October 24, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Death111 Death111 Death111 requested changes

Assignees

@MomdAli MomdAli

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MomdAli @QodoAI-Agent @Death111