Refactor/jpro 183 Refactor Add and Edit Description Forms into unified Controller

[vollbrecht-work]

The newly added DescriptionBaseController now holds all duplicated code that was
present in the AddFullyQualifiedNameController, AddOtherNameController,
EditFullyQualifiedNameController and EditDescriptionFormController.

Four new controllers were added that still holds the some controller specific's
from the old above Controllers. They now are called Description{Edit/Add}{Other/Fqn}Controller.

A unified FXML was introduced called description-form.fxml, to reduce copy pasted
view's, helping reduce the UI bug surface.

[vollbrecht-work]

Just some comments from me to clarify why i did certain things. Feel free to question everything ;D

[vollbrecht-work]

I dont think this constructor is used, it was there before so we can get rid of it i think.

[carldea]

yes remove it! 👍🏼

[vollbrecht-work]

This is the only InjectedViewModel of type OtherNameViewModel, we could further reduce duplicated code by streamlining all 4 Controllers to a single ViewModel. The three other Controllers are using the non wrapped ViewModel

[carldea]

Hmm...
I created a common view model called dev.ikm.komet.kview.mvvm.viewmodel.DescrNameViewModel. Might need updating, but that was my original attempt to consolidate the description name pages.

I don't like the OtherNameViewModel because a regular name and a fully qualified name have the same kind of data. Other name is the same as when you hear us say Regular name. Description Semantics contains fields like language, text, case significant, description type. Description type is one of three concepts.

• FQN
• RegName
• Definition.

Concept window allows you to add, edit fqn and regname. No windows (bump outs) allow you to update definition.

edit is an overloaded term. In Komet/db no record is every deleted. There is only add. edit is adding a new version derived from the previous version.

Mvvm with Cognitive
ViewModels - contain two layers. Property values and Model values. Properties are usually bound to UI controls either bidirectionally or uni directional. Save() method copies values from prop values to model values. Reset() will copy in the other direction. Validation view model's save() method will call validate() and if successful the copy continues, but if it fails it does not copy. this allows a ui to perform clear for create screens and reset for edit screens. Keep an eye on the constructor of the ViewModel.

Controllers: who owns the bump out pane re: Descr Name?
In the DetailsController i believe it's nested where the toggle to open the right side will be the PropertiesController. And again another layer of indirection it (property controller) owns all the Pane(s) to show such as Add fully q name or edit other name.

[vollbrecht-work]

edit is an overloaded term. In Komet/db no record is every deleted. There is only add. edit is adding a new version derived from the previous version.

Ah ok thanks got it.

Mvvm with Cognitive ViewModels - contain two layers. Property values and Model values. Properties are usually bound to UI controls either bidirectionally or uni directional. Save() method copies values from prop values to model values. Reset() will copy in the other direction. Validation view model's save() method will call validate() and if successful the copy continues, but if it fails it does not copy. this allows a ui to perform clear for create screens and reset for edit screens. Keep an eye on the constructor of the ViewModel.

Yeah i got that part, its just that it seams this old controllers did not adhere fully to this nice separation. One question with this wired hasOtherName boolean/ propertie of the OtherNameView Controller. What they currently try to model with that is i think setup comboxes, by moving a DescNameView into a OtherNameView and if that original Concept has no OtherNames associated with it than it should updateView the comboboxes like this caseSignificanceComboBox.setValue(TinkarTerm.DESCRIPTION_NOT_CASE_SENSITIVE); otherwise use what already set as a Propertie.

[vollbrecht-work]

not sure if this approach is liked by you guys. I just go with it for simplicity. If you rather want to see a edit-form and a add-form than i could do that, but i like to not have 4 forms that are essentially the same, and are just copy pasted around. That otherwise just opens the UI bug surface unnecessarily

[carldea]

I would want one form. The DescrViewModel extends from FormViewModel and contains a property called MODE which is either in Create or Edit mode. So you could use that to differenciate how to display the title.

Mvvm with Cognitive:
There are three ways to interact with the view model.

• Before calling FxmlMvvmLoader.make()
• After
• Inside of a JavaFX controller

Config config = new Config(...);

/// [Before] Update view model before Node and controler is created.
config.updateViewModel( "myViewModel", myViewModel -> {
    myViewModel.setPropertyValue(SOME_TITLE, "hello!!");
});
JFXNode<Node, MyController> jfxNodeController = FXMLMvvmLoader.make(config);

// [After] Update view model after Node and controller is created.
jfxNodeController.updateViewModel( "myViewModel", myViewModel -> {
    myViewModel.setPropertyValue(SOME_TITLE, "good bye");
});

[In Controller] When interacting inside of the Controller:

@InjectViewModel
MyViewModel myViewModel;

@FXML
Label someTitle;

@FXML
private void initialize() {
     someTitle.textProperty().bind(myViewModel.getStringProperty(SOME_TITLE));
}

private void changeTitle(String title) {
   myViewModel.setPropertyValue(SOME_TITLE, title);
}

[vollbrecht-work]

ah thanks thats good info you are given me here. So if we already have a MODE property, i can use that to get rid of the DescriptionFromType. The only other thing i have to investigate is

Getting rid of otherNameViewModel

in the following code snippet that's inside the initialize() part of the PropertiesController. The otherNameViewModel gets its "custom properties set" in a event subscriber.

                // Create a new View Model for this form
                addOtherNameController.updateModel(getViewProperties(), (viewModel, controller) -> {
                    // Clear view model. Please see addOtherNameController's initialize() method.
                    viewModel.setValue(NAME_TEXT, "")
                            .setValue(CASE_SIGNIFICANCE, null)
                            .setValue(MODULE, null)
                            .setValue(LANGUAGE, null)
                            .setValue(STATUS, TinkarTerm.ACTIVE_STATE)
                            .setValue(IS_SUBMITTED, false)

                            .setValue(FQN_CASE_SIGNIFICANCE, addFullyQualifiedNameController.getViewModel().getValue(CASE_SIGNIFICANCE))
                            .setValue(FQN_LANGUAGE, addFullyQualifiedNameController.getViewModel().getValue(LANGUAGE))

                            .setValue(HAS_OTHER_NAME, hasOtherNames);

our goal is to get rid of the last 3 prop's because they make up this wired OtherNameViewModel. The first two seams simple as they are just transfered from the extended DescNameViewModel. The last HAS_OTHER_NAME is currently just a boolean tracked inside the PropertiesController. I am not sure where this prob is currently used.
- if its used: i would "as a stopgab" add as a prop into DescNameViewModel
- if its unused: well everything good then and we can just directly nuke OtherNameViewModel.
Its used i talk about it down below in the last part

consolidating into a single reusable FormController

some road bumps we would need to consider.

• The BaseController is generic because the contained ComboBoxes are either of inner ConceptEntitiy or EntitiyFacade. The generic itself is for now not the biggest problem but if you could get me a rundown on how this ConceptEntity and EntitityFacade are playing together, also in the overall view of what you said in the other comment that in komet things are only added, and in an edit its a "derived add". This comment is also with respect to the two "similar" setupComboBoxAdd/Edit methods that may can be consolidated.
• If i understand correctly validation should only be part of the ViewModel and not the view right? The two old edit controllers still did some validation inside the controllers itself, so model that into the ViewModel.
• To not change to many logic in one go i would move all public methods of the specific 4 controllers into the BaseController, and rename the methods to indicate where they came from. Than we see how that would look like and can follow up on that in a PR after it to fully unify? Would that work for you?
• The info currently if we are a Add/Edit is stored not as a properties but as a boolean state value of the PropertiesController. This values than is set in this code

        ObservableList<DescrName> otherNames = getConceptViewModel().getObservableList(OTHER_NAMES);
        otherNames.addListener((InvalidationListener) obs -> {
            if (!otherNames.isEmpty()) {
                propertiesController.setHasOtherName(true);
            }
            updateOtherNamesDescription(otherNames);
        });

via the setHasOtherName in the DetailsController. With that background for now is it the right idea to make it a property if its "a other name" or not? In the current code, there already exist a OTHER_NAMES property inside the ConceptViewModel and that is holding all different descriptions. This hasOtherName is used in updateView() method of the AddOther controller to ask get the comboBoxes filled either with data from the viewModel or get a "default" set that comes from TinkarTerm ? Here is the code

        if (hasOtherName) {
            caseSignificanceComboBox.setValue(getViewModel().getValue(FQN_CASE_SIGNIFICANCE));
        } else {
            caseSignificanceComboBox.setValue(TinkarTerm.DESCRIPTION_NOT_CASE_SENSITIVE);
        }
        statusComboBox.setValue(Entity.getFast(State.ACTIVE.nid()));
        moduleComboBox.setValue(TinkarTerm.DEVELOPMENT_MODULE);
        if (hasOtherName) {
            languageComboBox.setValue(getViewModel().getValue(FQN_LANGUAGE));
        } else {
            languageComboBox.setValue(TinkarTerm.ENGLISH_LANGUAGE);
        }

The above code was the justification for adding the OtherNameViewMode ? So if we can alter this logic it would be simple to get rid of it. Do you have a particular wish how we should structure it? I can than just do it.

Summarize questions

• Controller stay generic yes/no? ConceptEntity vs EntityFacade.
• should i directly alter logic? Otherwise for now add a hasOtherName prop and on a follow up remove it? DetailsControler sets it -> PropertiesController saves it as boolean currently -> DescriptionController in the "add other" case will run code above to fill comboboxes depending on that value.
• move all the stuff into the DescriptionBaseController and rename colliding public methods adding a prefix where they come from, then call them from the PropertiesController? This intermediate step can than be followed with a more streamlined refactor of the logic, but let us move now?

[carldea]

I can provide more answers next week.

The large if/then above may need others to weigh in on what that's all about. Might have been for default values not sure.

Some more notes:
Older Create/Edit panels may have better logic for comboboxes. They are populated with a function called decendantsOf(PublicId publicId). Also all EntityFacade objects have a method called publicId() and nid(). Think of a nid as a database id, and the public id as a universal UUID for components(Concepts, Patterns, Semantics, and Stamps). Check out the constructor of a view model and see how available option (items) are populated.

decendentsOf() is only for concepts having children in the concept navigator. Not all comboboxes use the decendentsOf().

[vollbrecht-work]

this two comboBox methods Add / Edit are quite similar. Could also be better refactored. I just didn't want to do to much in one go.

[carldea]

yes, we should becareful. Ideally i don't mind one go, but they all have slightly different things. The newer windows (dev.ikm.komet.kview.mvvm.view.pattern.PatternDetailsController/PropertiesController) really look more like the figma designs and validation and functionality is more correct (in terms of look and feel). Just take it slow and take notes. What ever you change, will need to be tested.

Or you can get what you can in this PR and a new PR for each place that makes use of the reusable controller.

[vollbrecht-work]

@carldea i hope this PR is not to big, as my comments above suggest we could go even further, but in the interest of progress i put this out.

[carldea]

I am not a fan of inheritance in this case. I also like JavaFX controllers to not have constructors like you see below.
Information can be passed into the viewmodel. If you don't want to use properties. You can create instance variables inside with getters/setters if you prefer.

I think consolidation I meant a reusable Controller. I hope i didn't confuse more. You might want to glean from
https://github.com/ikmdev/komet/blob/main/kview/src/main/java/dev/ikm/komet/kview/mvvm/view/common/StampAddController.java

[vollbrecht-work]

yeah it was just a stopgap solution to massage the code into the right direction, test if it still work, and getting feedback. I created the 4 Controllers to have a simpler transition to the end goal of having only 1 reusable Controller and a single ViewModel.

I put some questions in the other comment and than we can go from there.

[carldea]

@vollbrecht-work
Can you put this in draft mode? I believe this is quite a bigger effort and would like this to take more time to refactor.

[carldea]

@carldea i hope this PR is not to big, as my comments above suggest we could go even further, but in the interest of progress i put this out.

Oh boy, I might have said to much or we (you and me) fell into a little bit of scope creep. :-(

@vollbrecht-work In short, (not to jerk you around), but is it possible to create another PR that just resolves the current ticket JPRO-183 where the buttons aren't layout responsive?

We can keep this PR (or create a new PR) for the consolidation work in draft. Because consolidation and refactoring touches too many files to risk at this time. This would be a work in progress.

Sorry to inform you.
Carl

[vollbrecht-work]

@carldea if you want that i can do that. Still would be nice if you can get me a headsup on the 3 summarize question. So i can in parallel look into it, Overall this refactor already did help me understand the current state of the code.

[carldea]

did help me understand the current state of th

Excellent. I like how you are thinking and understanding the effort. I really wish we had a spike ticket, to evaluate and compare.
I got very busy. So i will try to reply to the other questions. If not we can work on this after our holiday. Be back on Tue.

Quick thought:
The data often will have EntityFacade, or EntityProxy, or Entity, all of them will implement various interfaces. Concrete class from the database are like ConceptRecord, PatternRecord, SemanticRecord, StampRecord. but all types of objects have a method called nid() or an interface called ComponentWithNid or EntityFacade. I believe sticking to the abstract ComponentWithNid or EntityFacade is the way to go. To avoid class cast exceptions.

Typically combo boxes should be like the following:
ComboBox<EntityFacade> items = ..
or
ComboBox<ComponentWithNid> items = ..

Of course there are many places that don't use the abstract interfaces and use concrete such as:
ConceptEntity, or PatternEntity.

Another thing to note:
Items in the combo box (concepts/EntityFacade) as shown as an available item to choose the combobox's valueProperty() gets set accordingly. But if you programmatically set value to an entity that isn't in the available list (during edit mode) the combobox look blank or unselected. This is because the data is the same but the objects are different.

A contrived example:
new Entity(123).equals(new Entity(123)) == false.

These are some of the not so obvious issues developers encounter often. Is when updating the view model and when fields bound to controls and properties they ask why isn't the item selected.