Open PreferencesFX for extension

[aalmiray]

The current design of the PreferencesFX class does not provide extension points that could be used to tweak behavior (see #4 for example). It would be better perhaps to expose internal state via protected fields or methods, also use the template factory method in the constructor, delegating the actual creation on collaborators to protected methods, such as

  protected PreferencesFx(Class<?> saveClass, Category... categories) {
    // asciidoctor Documentation - tag::testMock[]
    preferencesFxModel = createPreferencesFxModel(
        createStorageHandler(saveClass), createSearchHandler(), createHistory(), categories
    );
    // asciidoctor Documentation - end::testMock[]

    // setting values are only loaded if they are present already
    preferencesFxModel.loadSettingValues();

    undoRedoBox = createUndoRedoBox(preferencesFxModel.getHistory());

    breadCrumbView = createBreadCrumbView(preferencesFxModel, undoRedoBox);
    breadCrumbPresenter = createBreadCrumbPresenter(preferencesFxModel, breadCrumbView);

    categoryController = createCategoryController();
    initializeCategoryViews();
    // display initial category
    categoryController.setView(preferencesFxModel.getDisplayedCategory());

    navigationView = createNavigationView(preferencesFxModel);
    navigationPresenter = createNavigationPresenter(preferencesFxModel, navigationView);

    preferencesFxView = createPreferencesFxView(
        preferencesFxModel, navigationView, breadCrumbView, categoryController
    );
    preferencesFxPresenter = createPreferencesFxPresenter(preferencesFxModel, preferencesFxView);
}

[martinfrancois]

Hi @aalmiray, thanks for your valuable comment!

Concerning #4, we discussed this already before making it public and our rationale was the following: In the use case of developers wanting to handle the storage of the values themselves, we thought that those would most likely already have their storage solution implemented and wouldn't want to bother with implementing an own StorageHandler and conforming to its interface. However, it turned out we were wrong. And that's a good thing! 👍 It's always hard to judge in advance what developers will need and what will be unused API which doesn't make sense to maintain. In this case, I'm glad the community responded with the demand and someone took the effort and implemented it. That's what open source is all about and what I really enjoy about this community!

Concerning the extension points in general. Yes, of course we could make everything entirely replaceable, but again, to be honest, we didn't expect there would be the need to do so. We thought if people had the need to customize anything (besides the things we expose already) that cannot be done purely with CSS, their use case would probably be so complex that they probably would want to implement their own preferences solution anyways and wouldn't want to bother with extending PreferencesFX. However, I would not mind getting proven wrong again here 😉 if you have a convincing use case, I would happily merge a PR implementing it! 😃

[martinfrancois]

Since there was no response, I will close this issue. Feel free to open it up again or create another issue should you still run into issues.

[aalmiray]

I'd keep this open. I was waiting for a release of FormsFx to be posted before embarking on opening up PreferencesFX for extension, as the latest codebase from FormsFX enables a lot of new features.

[martinfrancois]

@aalmiray thanks for letting us know! Makes sense, I just reopened it.

[sdp0et]

if StorageHandlerImpl used PreferencesFactory.userRoot() to get an instance of Preferences instead of preferences = Preferences.userNodeForPackage(saveClass);

wouldn't this allow a user to provide a custom Preferences/PreferencesFactory implementation without needing to create a new StorageHandler implementation that essentially duplicates it but with the custom Preferences implementation?

edit: nevermind. When I was trying to set the factory system property I was also using the classname as they key, so the default WIndowsPreferencesFactory was being used, and I didn't read deep enough before assuming that userNodeForPackage() ignored the setting and just used the default. I see now that this is exactly what's happening.