bamaer commented on PR #8313:
URL: https://github.com/apache/hop/pull/8313#issuecomment-5632388185
Two points on the change itself. Everything else that came up while reading
this is pre-existing and will go into follow-up tickets rather than hold up
this PR.
### 1. `notifyGuiAfterImport` has no effect in the folder-import case
`KettleImportDialog.java:805-812` fires `MetadataChanged` and
`ProjectUpdated`. Both listeners refresh the **active** project:
- `MetadataChanged` -> `MetadataPerspective.java:368`, `e -> refresh()`,
which reloads from `hopGui.getMetadataProvider()`
- `ProjectUpdated` -> `ExplorerPerspective.java:477-479`, `e -> refresh()`
In the folder-import branch (`KettleImportDialog.java:687`) the import
writes files and metadata into the newly registered project's folder, and —
deliberately, as the whole point of this PR — that project is never activated.
The active metadata provider and the explorer root still point at the original
project, so both events rebuild exactly the content that was already on screen.
So the third bullet of the description ("Fires `MetadataChanged` and
`ProjectUpdated` after a successful import so the metadata perspective
(including the category tree) and explorer pick up files written to disk") does
not hold for the case the issue is about. The same applies less obviously to
import-into-existing (`:698`): the target is whichever project the user picked
in the dropdown, which is not necessarily the active one, so the refresh helps
only where those happen to coincide.
Two workable directions:
- Drop `notifyGuiAfterImport` and its two tests. Nothing is lost for the
folder case, and the PR gets smaller and easier to reason about.
- Keep it, but fire only when the import target is the active project, and
reword the comment at `:775-779` and the description bullet to say that.
Either is fine; leaving it as-is means the description promises a refresh
that does not happen.
### 2. `Const.NVL` does not cover an empty `defaultProjectConfigFile`
`HopImportCreateProjectIfNotExists.java:65-68` substitutes the default only
on `null`. `defaultProjectConfigFile` is an editable `TextVar` in the projects
options (`ProjectsConfigOptionPlugin.java:404`) and an empty value passes the
`!= null` guard at `:281`, so `""` is reachable. It then reaches
`projectHome.resolveFile("")` at `:75`, which returns the project home folder
itself, and `project.saveToFile()` at `:86` tries to write the config over a
directory.
This is new here only as a side effect of a change that is otherwise an
improvement: before this PR the `ProjectConfig` got the configured name while
the file on disk was hardcoded to `project-config.json`, so the two could
disagree. Unifying them is the right move — the empty case just needs the same
fallback `ProjectConfig.getActualProjectConfigFilename` already uses at
`ProjectConfig.java:140-142`:
```java
String defaultProjectConfigFilename =
variables.resolve(config.getDefaultProjectConfigFile());
if (StringUtils.isEmpty(defaultProjectConfigFilename)) {
defaultProjectConfigFilename =
ProjectsConfig.DEFAULT_PROJECT_CONFIG_FILENAME;
}
```
Same root cause in the test:
`HopImportCreateProjectIfNotExistsTest.java:108` asserts on the literal
`project-config.json`, but `HopConfig.setInMemoryMode(true)` in `setUp` runs
after `HopConfig` has already read the developer's `~/.hop/hop-config.json`, so
the code under test uses whatever `defaultProjectConfigFile` that machine has.
On an environment where it is set to `hop-project.config` (one of the
`CONFIG_FILENAME_CANDIDATES`) the assertion fails. Setting the value explicitly
on the fixture's `ProjectsConfig` makes the test independent of the host
configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
