Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove lock to delete or rearrange process links #4850

Closed
wants to merge 5 commits into from

Conversation

matthias-ronge
Copy link
Collaborator

Fixes #3275

Copy link
Member

@solth solth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes break the existing functionality and are insufficient to fix the linked issue. Just allowing the user to click linked processes in the metadata editor does not work without actually adding functionality to tell the system what to do with selected linked processes.

Without that new functionality, with just these changes, selecting a linked process in the metadata editor leads to IllegalStateExceptions like the following:
Bildschirmfoto 2021-12-02 um 10 44 37
@matthias-ronge did you actually test your changes?

@matthias-ronge
Copy link
Collaborator Author

I only removed the block because we had a customer complaint that it was not possible to delete or move links. I had built in these functions two years ago and they worked at the time. I admit that I am amazed at the mistake. There should actually be no error. For example, you could give this element metadata—whether that is intentional or makes sense is another question. It's good that I'll look it again.

@matthias-ronge
Copy link
Collaborator Author

this is the code where the IllegalStateException is thrown (the last line).

/**
 * Determine and return StructureElementViewInterface corresponding to structure element currently selected or to be
 * added via AddDocStructTypeDialog.
 *
 * @param dataEditor DataEditorForm instance used to determine StructureElementViewInterface
 * @return StructureElementViewInterface corresponding to structure element currently selected or to be added
 */
public static StructuralElementViewInterface getStructuralElementView(DataEditorForm dataEditor) {
    Optional<LogicalDivision> selectedStructure = dataEditor.getSelectedStructure();
    if (selectedStructure.isPresent()) {
        return dataEditor.getRulesetManagement()
                .getStructuralElementView(
                        selectedStructure.get().getType(),
                        dataEditor.getAcquisitionStage(), dataEditor.getPriorityList());
    } else {
        TreeNode selectedLogicalNode = dataEditor.getStructurePanel().getSelectedLogicalNode();
        if (Objects.nonNull(selectedLogicalNode)
                && selectedLogicalNode.getData() instanceof StructureTreeNode) {
            StructureTreeNode structureTreeNode = (StructureTreeNode) selectedLogicalNode.getData();
            if (structureTreeNode.getDataObject() instanceof View) {
                View view = (View) structureTreeNode.getDataObject();
                if (Objects.nonNull(view.getPhysicalDivision())) {
                    return dataEditor.getRulesetManagement().getStructuralElementView(
                        view.getPhysicalDivision().getType(),
                            dataEditor.getAcquisitionStage(), dataEditor.getPriorityList());
                }
            }
        }
    }
    throw new IllegalStateException();
}

For me this is spontaneously confusing, because the selected structure should be given, right? Didn't you select anything when you selected the link? Or is the selection not considered selected by JSF? I am spontaneously at a loss.

@matthias-ronge
Copy link
Collaborator Author

I am not experiencing the exception you reported. For me, this works as expected.

@matthias-ronge
Copy link
Collaborator Author

I checked in the debugger: When I click the link, the function is called, but selectedStructure.isPresent() is true and so the if-condition applies, no error here. In your case it must be false for some reason which doesn’t make sense if you actually selected something, so this actually is an illegal state, but the question is why.

@solth
Copy link
Member

solth commented Dec 9, 2021

I think I found out why you couldn't reproduce the error. It only occurs for me when I select parent processes in the structure tree, but not when I click on child processes:

Bildschirmvideo.aufnehmen.2021-12-09.um.10.28.07.mov

@solth
Copy link
Member

solth commented Dec 9, 2021

Additionally, I think something is not updated correctly when removing a linked child process in the metadata editor structure tree using the "Remove element" context menu option.
When expanding the row for the parrent process in the process list afterwards, it still displays all previously linked child processes, including the one I just removed in the editor (I didn't forget to save in the editor, the removed child process is not shown again in the editor when opening the parent process again!)

@matthias-ronge
Copy link
Collaborator Author

Thanks for the hint. I'll take a closer look at that. If deleting doesn't work anymore, that's a bug.

@matthias-ronge
Copy link
Collaborator Author

I fixed the initial problem (the crash). I cannot reproduce the “updating” problem. Either I misunderstand it, or it was a side effect of the other problem and is now gone:

Animation

I would suggest to handle this in a separate issue if it still happens.

@matthias-ronge matthias-ronge marked this pull request as ready for review January 17, 2022 11:07
@matthias-ronge matthias-ronge requested a review from solth January 17, 2022 11:07
Copy link
Member

@solth solth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still get the same error

@matthias-ronge matthias-ronge requested a review from solth January 18, 2022 07:49
Copy link
Member

@solth solth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception is now handled correctly and selecting linked processes does not result in an IlllegalStateException anymore. 👍

But there are still some problems that need to be resolved:

  • rearranging linked child processes via drag'n'drop results in wrong error messages (see first attached video below)
  • removing a linked parent process does not work (see second attached video below); if this is not supposed to work, the corresponding option in the context menu should be deactivated/hidden when selecting parent processes
  • context menu options Element hinzufügen, Typ des Strukturelements ändern and Medien hochladen do not work for linked processes and should be deactivated or hidden (see screenshots below)
rearrange-links-error.mov
remove-link-to-parent.mov

Bildschirmfoto 2022-01-18 um 10 12 43

@matthias-ronge
Copy link
Collaborator Author

A rebase is no longer possible because the diff is no longer understandable. Development is terminated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process link cannot be deleted
2 participants