Repository navigation
#2589: Inject an application-level IdeContext into NlsService from th… - #2648
krystynaShatkovska wants to merge 7 commits into
Conversation
…from the GUI The GUI previously constructed NlsService with a null context, which made NlsService fall back to lazily creating its own IdeContextConsole at persistence time -- a service reaching into the environment to build its own dependency. Inject an application-level IdeContextConsole bound to the GUI's start context from GuiStateManager, and remove the now-unused NlsService(Locale) constructor and the fallback creation. NlsService now always receives its context via dependency injection. This also fixes a latent issue: the old fallback built its own start context and discarded the GUI logging context. Locale handling and IDE_OPTIONS isolation behavior are unchanged.
krystynaShatkovska
added
enhancement
Coverage Report for CI Build 38900158726Coverage decreased (-0.002%) to 74.753%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions29 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
laim2003
left a comment
laim2003
left a comment
There was a problem hiding this comment.
Thank you @krystynaShatkovska. The implementation is now much better and less hacky. However, the persistence does not yet work and also does not seem to be implemented in the code currently, but at the same time that is also not the scope of this PR. I'll check if we already have a PR that fixes this issue and if not I will create one. Regardless in my opinion this PR is ready to merge.
quando632
left a comment
quando632
left a comment
There was a problem hiding this comment.
Thanks for finishing this refactoring. NlsService now has a single constructor with Objects.requireNonNull, so it no longer creates a context behind the scenes. Moving the IdeStartContextImpl creation up and binding the console context to it keeps the GUI log listener, which is a good detail. The updated Javadoc also explains clearly how production code and tests supply the context.
|
The description says |
Address review feedback: GuiStateManager now offers an (ideRoot, IdeContext) overload; the single-arg constructor delegates and derives the context from the GUI start context. AppBaseTest injects an isolated IdeTestContext (stub System, throw-away user home) so persisting the locale no longer rewrites the developer's real ~/.ide/ide.properties.
Resolve conflicts from upstream's GUI rework (devonfw#2480 Rework GUI tests, devonfw#2400 Rework console) against the devonfw#2589 NlsService context injection: - NlsService keeps the single (IdeContext, Locale) constructor; the lazy IdeContextConsole fallback is removed. - GuiStateManager keeps the (ideRoot, IdeContext) overload and the startContext-first ordering, dropping the now-removed ConsoleController. - AppBaseTest now extends UIBasedApplicationTest and injects an isolated NlsService context (throw-away @tempdir user home) so setLocale never rewrites the real ~/.ide/ide.properties. - ConsolePanelTest (new in the console rework) is migrated off the removed single-arg NlsService constructor to the injected-context form.
quando632
left a comment
quando632
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. The new GuiStateManager(String, IdeContext) overload keeps the production default intact, and AppBaseTest no longer writes to the real ~/.ide/ide.properties. Looks ready to merge from my side.
4 participants
This PR fixes #2589
In PR #2548, NlsService was made to accept an injected IdeContext so its tests could run in an isolated environment, but the GUI kept using the old no-context path and NlsService retained a fallback that lazily created an IdeContextConsole when no context was present. This finishes the refactor so the context is always supplied through dependency injection:
Implemented changes:
Change 1
NlsService no longer creates a fallback IdeContext. It now has a single NlsService(IdeContext, Locale) constructor that requires a non-null IdeContext. The NlsService(Locale) constructor, which left the context null and forced the fallback to a fresh IdeContextConsole, is removed.
Change 2
The GUI supplies the context. GuiStateManager now constructs an application-level IdeContextConsole bound to the app start context and injects it into NlsService. Binding it to the start context also keeps the GUI logging context, which the old no-arg IdeContextConsole() discarded.
Change 3
Behavior is preserved. Locale persistence still writes IDE_OPTIONS = -Duser.language= to the user-scope ~/.ide/ide.properties, and the IDE_OPTIONS isolation behavior introduced in #2533: isolate NlsServiceTest and stop IDE_OPTIONS env var from pollu… #2548 is unchanged.
Change 4
NlsServiceTest is updated to inject the isolated test context instead of the removed constructor.
Testing instructions
mvn -pl gui -am test -Dsurefire.failIfNoSpecifiedTests=false -Dtest='NlsServiceTest,AppBaseTest' -> NlsServiceTest (29) and AppBaseTest (10) pass. A full mvn verify also passes.
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat).If no issue ID exists, title only.
In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed,you have updated our LICENSE.adoc