Feature/finished/iia 2027 resolve create semantic and fixes #478

carldea · 2025-05-19T22:37:07Z

This branch contains.

fix to exception when creating a semantic
fix to use window topic instead of journal topic
support many dnd sources such as original concept nav

…-fixes

dukke

@carldea Just had a quick look as it's a bit late here :) Added 1 comment from that very brief look.

kview/src/main/java/dev/ikm/komet/kview/controls/skin/KLComponentControlSkin.java

application/pom.xml

dukke · 2025-05-20T11:28:05Z

kview/src/main/java/dev/ikm/komet/kview/mvvm/view/genediting/GenEditingDetailsController.java

+        ObjectProperty<EntityFacade> refComponent = genEditingViewModel.getObjectProperty(REF_COMPONENT);
        //Enable edit fields button if refComponent is NOT null else disable it.
-        editFieldsButton.setDisable(refComponent == null);
+        editFieldsButton.setDisable(refComponent.isNull().get());


We can do a bind here:
editFieldsButton.disableProperty().bind(refComponent.isNull());
And then I think we won't need to manually update it anymore below.

Good call! We should create another ticket since this is merged already.

@carldea yes, a ticket is a good idea! Do you want me to create it?
There are a bunch of places perhaps we should clean up.. including my code (for instance KLComponentSet control is still in the repo but we don't need it anymore).
Perhaps we could take some time after the demo to reduce some of our technical debt?

dukke · 2025-05-20T11:28:49Z

kview/src/main/java/dev/ikm/komet/kview/mvvm/view/genediting/GenEditingDetailsController.java

+        editFieldsButton.setDisable(refComponent.isNull().get());
        //Enable reference component edit button if refComponent is NULL else disable it.
-        addReferenceButton.setDisable(refComponent != null);
+        addReferenceButton.setDisable(refComponent.isNotNull().get());


We can do a bind here:
addReferenceButton.disableProperty().bind(refComponent.isNotNull());
And then I think we won't need to manually update it anymore below.

dukke

Added some more comments.

I appreciate the commented out text saying we would get the database objects out of this control later. We share the same concerns. I understand that there's a time pressure to get this done before the demo. I hope we have time to do it later :)

dukke · 2025-05-20T11:31:49Z

framework/src/main/java/dev/ikm/komet/framework/view/ViewMenuTask.java

+                if (parentMenu != null) {
+                    parentMenu.getItems().clear();
+                    TinkExecutor.threadPool().execute(TaskWrapper.make(new ViewMenuTask(viewCalculator, observableView),
+                            (List<MenuItem> result) -> parentMenu.getItems().addAll(result)));


Is this menu already attached to a Scene? If so we'd want to use Platform.runLater to execute the code that changes the UI in the UI thread

@carldea are we okay with editing this since it is shared code for the classic Komet's edit coordinate window in addition to the "read only for demo" one we generate for the Concept window...???

I am late getting these messages. It's too late in the game. But next time small stuff like this feel free to fix as a shared branch.
I agree with @dholubek this is read only for the demo and not to be invoked (trigger actions). But I believe the code was really lifted from classic Komet. Ideally when using a thread pool it provides activity information in the Activities tab for long running loads (building many menu options).

I think @dukke is correct that the parent (context menu) is attached to a Scene, and when adding to it (parentmenu) should be done on the JavaFX thread.

@carldea sounds good. Do you want me to add this to the ticket we mentioned?

dukke · 2025-05-20T11:48:30Z

kview/src/main/java/dev/ikm/komet/kview/mvvm/view/genediting/SemanticFieldsController.java

+
+                   try {
+//                       EntityService.get().endLoadPhase();
+                       createSemanticVersionTransactionTask(transaction).get(); // get() will block until transaction is finished (will refresh view calculator caches)


We are inside the UI thread and we are blocked until get() returns, this will block the UI thread until the task is complete, and the UI will become unresponsive.
We could spawn a new thread. Inside that thread we run this code and after we have the results call all the code that updates the UI inside Platfrom.runLater

@dukke This is what we want, we would want the thread to complete before we send response. This is why we are getting the exception on create.

Pedro is correct in that we should never block like this, but while fixing this issue we needed it to block in order to know diagnose why/when a transaction is finished creating a semantic. Swaroop was half/correct (from yesterday), but because of the fix in tinkar core we'd need to put the async code back in (surrounding the code above).

Let me explain.
Using a small dataset, the response time is negligible(fast), however if you use the snomed data, the creation code takes a very long time. Yesterday we found out the issue was code in the database code was asynchronous and we couldn't predict when the transaction was actually complete and the left side would query too soon and retrieve data from a stale cache.

Once we fixed that issue hence tinkar-core 1.103.0. I forgot to add the Completable futures code back in to not freeze the GUI. :(

@carldea Yeah now that you mentioned it, I do remember that in the code you posted on Teams in our group chat you were indeed creating a new thread and calling this code inside that thread.
I did a small test where I simulated a long running call to the database (using thread sleep and having the code be the same as in this PR) and indeed the UI completely froze.

I have a quick fix idea for this if you'd like me to share it.
We could change the createSemanticVersionTransactionTask method to have an additional parameter that would be a callback that is called when the transactions is finished. Something like this:

private Future<Void> createSemanticVersionTransactionTask(Transaction transaction, Runnable runAfterTransaction) { CommitTransactionTask commitTransactionTask = new CommitTransactionTask(transaction); return TinkExecutor.threadPool().submit(new Callable<Void>() { @Override public Void call() throws Exception { commitTransactionTask.call(); runAfterTransaction.run(); return null; } }); }

Notice the runAfterTransaction runnable callback.

dukke · 2025-05-20T12:09:41Z

kview/src/main/java/dev/ikm/komet/kview/mvvm/view/genediting/GenEditingDetailsController.java

-                editFieldsButton.disableProperty().bind(newRefComponentProp.isNull());
-                addReferenceButton.disableProperty().bind(newRefComponentProp.isNotNull());
+                editFieldsButton.setDisable(newRefComponentProp.isNull().get());
+                addReferenceButton.setDisable(newRefComponentProp.isNotNull().get());


if we use binding above we won't need to update the disable property of the buttons again here

This was discussed and Carl specifically wanted to not have bind here. He has done this intentionally.

dholubek · 2025-05-20T12:38:49Z

kview/src/main/java/dev/ikm/komet/kview/controls/skin/KLComponentControlSkin.java

+        // Existing checks for next gen search navigator items to drag.
+        boolean nextGenSearchDnD =  isADragAndDropInfo(dragEvent);
+
+        return classicConceptNavigatorDnD || classicSearchDnD || nextGenConceptNavigatorDnD || nextGenSearchDnD;


@carldea this is fixing what we can drop into a component, which is good, but it has a broad scope because the component is used by the Semantic window and the Pattern window in several places. So the permutations of testing all this is broad with classic Komet and next gen search against those two window types and all the places inside them that use the component also during edit and create modes... are we sure we want to introduce that much extra scope this late? Do we want to just stick to the transaction related fix on the Semantic?

@dholubek @carldea I was thinking the same thing as Doug and I have to agree with him. :)
This is a valuable effort and it's good to have but I also think it's risky to introduce these new features at this stage (new drag and drop support).
We already post-poned 4 code line PRs that fixed bugs for the demo because we didn't want to risk it. This new drag and drop support introduces more than that and hence more risk and might not even be necessarily needed for the demo.
Would sticking to the semantic creation bug fix be better?

dholubek

approved... I made a few comments wrt to allowing both classic and next gen concepts into drag and drop and how that to me is a bit risky to add that scope this late... that is a concern of mine.

swaroopsalvi

Pros:

Fixing create semantic issue (Exception thrown during creation fixed).
Support for classic komet navigator drag and dorp and search -> Unit tested and ready.
Integration tinkar-core 1.103.0 -> performance related fix in tinkar-core.
Unblocks current sprint stories for updateVersion and semantic.

Cons:

This is a last min change and will need some additional testing.
It will freeze the UI while the save is in progress. (Side effect of the solution done.) -> Can be fixed.
One of JavaFx rules to have Platform.runLater() to be used to update the UI code.

carldea added 3 commits May 19, 2025 18:06

Update version to start feature: IIA-2027-resolve-create-semantic-and…

2d1db13

…-fixes

Merge branch 'main' into feature/IIA-2027-resolve-create-semantic-and…

f843900

…-fixes

Added additional dnd sources, fixes to gen editing window with submit

1922855

carldea requested review from dholubek and swaroopsalvi May 19, 2025 22:37

dukke reviewed May 19, 2025

View reviewed changes

kview/src/main/java/dev/ikm/komet/kview/controls/skin/KLComponentControlSkin.java Show resolved Hide resolved

dholubek reviewed May 20, 2025

View reviewed changes

application/pom.xml Show resolved Hide resolved

Added more comments updated pom files. refresh concept details view

51f9175

carldea requested review from aks8m and rbradley813 May 20, 2025 04:00

dukke requested changes May 20, 2025

View reviewed changes

dholubek reviewed May 20, 2025

View reviewed changes

dholubek approved these changes May 20, 2025

View reviewed changes

swaroopsalvi approved these changes May 20, 2025

View reviewed changes

swaroopsalvi merged commit e2c07fc into ikmdev:main May 20, 2025
6 checks passed

Feature/finished/iia 2027 resolve create semantic and fixes #478

Feature/finished/iia 2027 resolve create semantic and fixes #478

Uh oh!

Conversation

carldea commented May 19, 2025

Uh oh!

dukke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukke May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholubek May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukke May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholubek left a comment

Choose a reason for hiding this comment

Uh oh!

swaroopsalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dukke left a comment •

edited

Loading

dukke May 20, 2025 •

edited

Loading

dholubek May 20, 2025 •

edited

Loading

dukke May 20, 2025 •

edited

Loading