#2401: Rework Mainwindow

[samuelkos17]

This PR fixes #2401 #2402

Implemented changes:

This is quite a huge PR that completely reworked our GUI architecture, the most important changes:

• Reworked the GUI from "MVC" to MVVM
• Split the MainController into single modules (NavigationPanel, LauncherTab)
• Introduced Tabs
• Removed any logic from the .fxml files
• Introduced Services & Factories
• Refactored the folder structure of the GUI
  • Regarding the terminology of the new folder structure:
    • Factory: Creates ViewModels & Views while containing the necessary services to do so. Initialized in App
    • Service: Encapsulates business logic and access to data or functionality independently of the UI. Initialized in App
    • Control: A self-contained UI component of the MainWindow. Extracted frm MainWindowView to improve maintainability and seperation of concerns, where introducing a dedicated View/ViewModel split would not be worthwile.
    • View: The actual UI representation without business logic. Its responsibility is to display data nd information from the ViewModel through bindings.
    • ViewModel: Acts as a bridge between the View and the Services/Factory/Data layer. It maintains the current state and contains the UI-related logic.

The Refactoring-Work isn't done with this PR though, every functionality we had before (besides the language selection) is also present in the new GUI, but the GUI architecture is still not clean enough and there are a few hacks/smells in the code that still need to be reworked. In order to not bloat this PR even further the next tasks are listed here:

• Rework/Reintroduce language selection Overhaul language selection logic #2214
• Implement Alert & Dialog Factorys Implement AlertFactory for the GUI #2439
• Rework the console handling Rework Console to MVVM #2400
• Update GUI Docu Add documentation for contributing to the GUI #2107,
• Implement an EventBus/EventListeners, especially to remove the weird TabPane attaching in the TabFactory Consider adding an EventBus to the new GUI #2433

---

Testing instructions

Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:

1. Run ide gui using the local dev build and test if everything works.

---

Checklist for this PR

Make sure everything is checked before merging this PR. For further info please also see
our DoD.

• When running mvn clean test locally all tests pass and build is successful
• PR title is of the form #«issue-id»: «brief summary» (e.g. #921: fixed setup.bat and not feature/921 fixed setup.bat). If no issue ID exists, title only.
• PR top-level comment summaries what has been done and contains link to addressed issue(s)
• PR and issue(s) have suitable labels
• Issue is set to In Progress and assigned to you or there is no issue (might happen for very small PRs)
• You followed all coding conventions
• You have added the issue implemented by your PR in CHANGELOG.adoc unless issue is labelled
  with internal
• You have not changed any dependency in pom.xml files or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc
• You have formulated clear instructions on how to test your contribution under "Testing instructions"

[coveralls]

Coverage Report for CI Build 34589585841

Coverage increased (+0.2%) to 74.09%

Details

• Coverage increased (+0.2%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• 39 coverage regressions across 2 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

39 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
com/devonfw/ide/gui/App.java	30	0.0%
com/devonfw/ide/gui/context/GuiStateManager.java	9	78.26%

---

Coverage Stats

Relevant Lines:	18934
Covered Lines:	14660
Line Coverage:	77.43%
Relevant Branches:	8403
Covered Branches:	5594
Branch Coverage:	66.57%
Branches in Coverage %:	Yes
Coverage Strength:	3.3 hits per line

---

💛 - Coveralls

[laim2003]

Thanks for the deep rework! It looks way better already. The architecture is now much cleaner and allows for other features to be built on top. Regarding the TabFactory/general Tab architecture I had some optimization ideas that we should think about. But anyways good work!

[laim2003]

I think there might be a cleaner way to structure this. Right now the caller has to manually compose both the viewmodel and the view (e.g. in openLauncherTab()), and pass a title key that's really already owned by the viewmodel.

Instead, we could introduce a TabComponent interface (or abstract class) that encapsulates a tab and owns its own construction:

• getId() or getKey() - NLS key the tab is identified(?) by (replaces the separate titleKey parameter)
• getView() - returns the tab's Node, creating it on first call and attaching its viewModel.
• onBeforeOpenTab() - a hook run right before getView() is called
• onClose() - there might already be listeners for this in javafx, so this is just an inspiration
• onFocus() - same here?

This gives us a small, explicit UI lifecycle. It mirrors how Android handles reusable UI components ("fragments"), where the fragment owns creating its layout and viewmodel. I know I always talk about Android because I'm kind of used to it, but generally, I think just orienting a bit on how Android does thing UI-wise might not be bad for the GUI as they have quite a clean UI concept IMHO: Fragment.onCreateView. The caller would then just do tabFactory.open(someTabComponent) and stop threading NlsService/viewmodel construction through the call site.

This also allows us to optimize other areas:

• We could simply inject the ConsoleController and the NlsService into each TabComponent via the constructor.

Example how we implement this:

public abstract class TabComponent {

  public GuiStateManager guiStateManager;
  public CommandletService commandletService;
  public ConsoleController consoleController;

  public TabComponent(GuiStateManager guiStateManager, CommandletService commandletService, ConsoleController consoleController) {
    this.guiStateManager = guiStateManager;
    this.commandletService = commandletService;
    this.consoleController = consoleController;
  }

  public abstract TabView getView(NlsService nlsService);

  public abstract SimpleStringProperty getTabTitleKey();
}

Then the Launcher Tab content would be implemented by creating a custom LauncherTabComponent:

public class LauncherTab extends TabComponent {

  public SimpleStringProperty tabTitleKey = new SimpleStringProperty("key_launcher_tab");

  public LauncherTab(GuiStateManager guiStateManager, CommandletService commandletService, ConsoleController consoleController) {
    super(guiStateManager, commandletService, consoleController);
  }

  @Override
  public TabView getView(NlsService nlsService) {
    IdeLauncherViewModel viewModel = new IdeLauncherViewModel(guiStateManager, commandletService);
    IdeLauncherView launcherView = new IdeLauncherView(viewModel, nlsService);

    //do more stuff (possibly)

    return launcherView;
  }

  @Override
  public SimpleStringProperty getTabTitleKey() {
    return this.tabTitleKey;
  }
}

Then in TabFactory we can use these components like this while completely detaching them from the TabFactory:

  public void openLauncherTab() {
    open(new LauncherTab(this.guiStateManager, this.commandletService, this.consoleController));
  }


  public Tab open(TabComponent tabComponent) {

    Tab existing = findByTitleKey(tabComponent.getTabTitleKey().getValue());
    if (existing != null) {
      this.tabPane.getSelectionModel().select(existing);
      return existing;
    }

    Tab tab = new Tab(this.nlsService.get(tabComponent.getTabTitleKey().getValue()), tabComponent.getView(this.nlsService));
    tab.setUserData(tabComponent.getTabTitleKey().getValue());
    tab.setClosable(true);
    this.tabPane.getTabs().add(tab);
    return tab;
  }

@JoelAdbu maybe you can give your two cents on this as well

TL;DR : Introduce a TabComponent abstraction that owns its own view/viewmodel construction and lifecycle hooks. This moves composition logic from the caller into the tab itself, making the API simpler and better aligned with how frameworks like Android handle reusable UI components.

Edit: I see that TabView.java and TabViewModel already implement a similar pattern in the current code. However, this suggestion keeps the view and viewmodel composition closer together, avoiding the need for the caller to thread dependencies and manually wire them. So basically the use of TabComponent just extends what you already added in this PR.

[laim2003]

I just realised that my example is not perfect as well, of course TabComponent would not hold a class like CommandletService. That would rather be implemented by a subclass like LauncherTabComponent

[samuelkos17]

Thanks a lot for the suggestion and the work that you put in this Review. TBH i don't really see the issue with creating the ViewModel and the View seperately in the e.g. TabFactory. And I also believe that we can't generalize what each constructor of Views and ViewModel will need in the future. I feel like this would make everything more complicated than it has to be, but maybe I just don't quite understand what the value of this change would be. Could you maybe explain it further?

[laim2003]

I'm not too sure whether this VM is not creating more complexity than it solves. This seems to me like its adding an unnecessary layer: The whole point of a helper VM is that callers express intent through it. Instead, MainWindowViewModel reaches in and mutates the raw list directly as getItems() etc just pass on the Observables.

[samuelkos17]

I see what you mean, I'll look into it again and then answer this comment.

[samuelkos17]

The reason I implemented it is to improve readibility, if we would just write the properties into the MainWindowViewModel we would have 9 extra properties and their Getters in the ViewModel which would bloat it up

[laim2003]

Suggested change

	public class TabFactory {
	public class TabManager {

Maybe we can discuss whether the TabFactory isn't actually more like a manager class, especially since we have functions like isOpen() and focus()?

[samuelkos17]

You're right, the TabFactory currently isn't a factory per se but I intended for it to be one but made a mistake while designing it. It should actually only be a helper Class that only creates new Tabs and doesn't do anything else. The isOpen and focus() logic should be handled by the MainWindowView. The root cause for this problem is the way that the TabFactory currently handles the TabPane from the MainWindowView, but this will be removed in the future (#2433) so that the TabFactory will actually end up as a TabFactory and not act as a Manager as it currently does.

[laim2003]

Architecturally, it seems weird for this to be here, since this should be decided by the Tab that launches the commandlets. If we apply the new structure for the tabbing I explained in the comments above, we could potentially completely remove any reference to the CommandletService in the TabFactory and directly pass the CommandletService to the new LauncherTab instead. Then the LauncherTab can directly itself set the pre-launch action. LauncherTab could accept a reference to this ViewModel via a constructor parameter, although that also does not seem perfect... After we successfully implemented the EventBus in a later PR, we can also simply send an "OPEN_CONSOLE" event and let the ViewModel handle it directly without passing it as an argument.

[samuelkos17]

Yeah this is, as I mentioned in the top comment, not a clean implementation that's why I explicility mentioned the tabfactory-rework in #2433 and already made a cleaner solution while working that out. I guess we could discuss if we would prefer your rework suggestion here or the EventBus solution.

[laim2003]

We should probably add a ticket in the future to implement styling via CSS, i'm not sure if we have done that yet

[samuelkos17]

Yeah i agree

[laim2003]

See my comments about the new architecture.

[laim2003]

This is probably a left-over from my old tests right? Because generally, we should avoid using this helper method and rather use the fake project testing system that we use in all other components of IDEasy.

[samuelkos17]

I'll look into it

[samuelkos17]

Yeah it's still a leftover and thanks for pointing that out! Chhanging this here wouldn't be much of an issue, but in AppBaseTest the change would be bigger. The tests work now and to not further bloat this PR I would suggest to move this into a seperate issue "Rework GUI Tests".

[JoelAdbu]

IMHO, it would make sense to extract this FXML loading logic into FxHelper, as we already have very similar code in NavigationPanelControl and MainWindowView.

You could add two helper methods along these lines:

public static void loadFxml(Node root, String fxmlName, ResourceBundle resources) {

    loadFxml(root, fxmlName, resources, null);
  }

public static void loadFxml(Node root, String fxmlName, ResourceBundle resources, Callback<Class<?>, Object> controllerFactory) {

    FXMLLoader loader = new FXMLLoader(root.getClass().getResource(fxmlName));
    loader.setRoot(root);
    loader.setController(root);
    loader.setResources(resources);
    if (controllerFactory != null) {
      loader.setControllerFactory(controllerFactory);
    }
    try {
      loader.load();
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }

This would reduce code duplication and keep the FXML loading pattern consistent across the different views.

After introducing this helper in FxHelper, the following block could be simplified to:

Suggested change

	final FXMLLoader loader = new FXMLLoader(getClass().getResource(fxmlName));
	loader.setRoot(this);
	loader.setController(this);
	loader.setResources(nlsService.getResourceBundle());
	try {
	loader.load();
	} catch (IOException e) {
	throw new RuntimeException(e);
	}
	FxHelper.loadFxml(this, fxmlName, nlsService.getResourceBundle());

Please apply the same refactoring in the MainWindowView constructor and the NavigationPanelControl constructor as well.

[samuelkos17]

Thanks for the suggestion, I'll implement it. However I have a question, what is the controllerFactory that you mention in your suggestion?

[samuelkos17]

Hm I'm not sure if this can be applied to MainWindowView and NavigationPanelControl though since they add other stuff to the loader before loading