[doc] Add initial ADR for the selection dialog as a tree

[florianbarbin]

Pull request template

General purpose

What is the main goal of this pull request?

• Bug fixes
• New features
• Documentation
• Cleanup
• Tests
• Build / releng

Project management

• Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
• Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
• Have the relevant issues been added to the pull request?
• Have the relevant labels been added to the issues? (area:, difficulty:, type:)
• Have the relevant issues been added to the same project and milestone as the pull request?
• Has the CHANGELOG.adoc been updated to reference the relevant issues?
• Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
• In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

• Does the title of the commit contributing the ADR start with [doc]?
• Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

• Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
• Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

• Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

• Variables have a proper type
• Functions’ arguments have a proper type
• Functions’ return type are specified
• Hooks are properly typed:
  • useMutation<DATA_TYPE, VARIABLE_TYPE>(…)
  • useQuery<DATA_TYPE, VARIABLE_TYPE>(…)
  • useSubscription<DATA_TYPE, VARIABLE_TYPE>(…)
  • useMachine<CONTEXT_TYPE, EVENTS_TYPE>(…)
  • useState<STATE_TYPE>(…)
• All components have a proper typing for their props
• No useless optional chaining with ?. (if the GraphQL API specifies that a field cannot be null, do not treat it has potentially null for example)
• Nullable values have a proper type (for example let diagram: Diagram | null = null;)

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

• Are all the event handlers tested?
• Are the event processor tested?
• Is the business code (services) tested?
• Are diagram layout changes tested?

Architecture

• Are data structure classes properly separated from behavioral classes?
• Are all the relevant fields final?
• Is any data structure mutable? If so, please write a comment indicating why
• Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Please describe here the various use cases to test this pull request

• Has the Kiwi TCMS test suite been updated with tests for this contribution?

[AxelRICHARD]

Ok on my side

[AxelRICHARD]

Are you sure about that? What about an update of the subscription that will modify the depth of the tree?

[florianbarbin]

I looked at the TreePropertySection and it uses that kind of data structure to deal with that issue.
I did not understand the first time why we made this choice but when I started to implement this ADR I understood why: we can't build a recursive query without knowing the tree depth.

Are you sure about that? What about an update of the subscription that will modify the depth of the tree?

I'm not sure to understand: that would suppose to have the depth information updated and sent a new request with the same containment depth with

objects {
   ...
   objects {
  	...
  	objects {
    	etc.
        }
    }
}

[florianbarbin]

Note that it is what we doing for diagrams but I suppose that it makes sense to limit the node hierarchy:
(see in diagramFragment.ts)

nodes {
    ...nodeFragment
    borderNodes {
      ...nodeFragment
    }
    childNodes {
      ...nodeFragment
      borderNodes {
        ...nodeFragment
      }
      childNodes {
        ...nodeFragment
        borderNodes {
          ...nodeFragment
        }
        childNodes {
          ...nodeFragment
          borderNodes {
            ...nodeFragment
          }
          childNodes {
            ...nodeFragment
            borderNodes {
              ...nodeFragment
            }
            childNodes {
              ...nodeFragment
              borderNodes {
                ...nodeFragment
              }
            }
          }
        }
      }
    }
  }

[AxelRICHARD]

What about an update of the subscription that will modify the tree? You should also be able to keep the already expanded nodes expanded.

[florianbarbin]

IMO, keeping the expanded state locally to the user makes sense since we don't share the representation with others.

[sbegaudeau]

I've rebased the PR, I'll look into it

[sbegaudeau]

The reference of the ADR has been modified

[sbegaudeau]

I don't think that this can realistically ever be a viable option given the MASSIVE performance cost that this would create in any meaningful project.

[florianbarbin]

The idea is to let the possibility for the specifier to reveal in the tree the selectable elements.
That would avoid for the end user to unfold each tree item to search for a specific element.

[sbegaudeau]

And there's a difference between expanding some elements and expanding all elements (which would have a MASSIVE performance cost)

[sbegaudeau]

We won't re-invent a tree representation, open an alternate version of the dialog with a <TreeView /> inside and use the tree representation directly. It support more than enough features.

[sbegaudeau]

Can it be enough? How can you figure out how to display a simple list as a tree? We should probably hardcore a TreeDescription in the view converter for now

[florianbarbin]

I'm not sure to understand. For me, we will have an hard-coded TreeDescription but provider by an IEditingContextRepresentationDescriptionProvider ?

[sbegaudeau]

How will the specifier be able to configure that?

[florianbarbin]

For me, the specifier will only indicate by a boolean (a checkbox in the studio) in the SelectionDescription whether he want to display the selectable element as a list or as a tree.

[sbegaudeau]

You need to reuse a tree subscription instead of the raw selection subscription with its list of objects. I don't understand why this needs to be modified and why displayedAsTree should exist on both Selection and its description.

[florianbarbin]

Ok with that but I still have a problem with the "message"

I agree to avoid opening a subscription toward the SelectionRepresentation if the SelectionDialog is intended to represent selectable elements as a tree (in this case we will rely on the TreeView).

The message specified by the specifier is for now not interpreted but the SelectionDescription java pojo code relies on a messageProvider (Function<VariableManager, String>) so it could be interpreted in the future.

So in this case, I'm not sure what I should do to compute the dialog message which is common for both flat and tree representations.

Should I provide the displayAsTree and the message statically by passing them in the graphQL API :

instead of :

type SingleClickOnDiagramElementTool implements Tool {
  id: ID!
  label: String!
  iconURL: [String!]!
  appliesToDiagramRoot: Boolean!
  dialogDescriptionId: String
  targetDescriptions: [DiagramElementDescription!]!
}

We will have :

type SingleClickOnDiagramElementTool implements Tool {
  id: ID!
  label: String!
  iconURL: [String!]!
  appliesToDiagramRoot: Boolean!
  dialogDescription: DialogDescription
  targetDescriptions: [DiagramElementDescription!]!
}



type DialogDescription {
  id: String!
  parameters; [DialogDescriptionParamter!]!
}

type DialogDescriptionParamter {
  key: String!
  value; String!
}

[sbegaudeau]

We will use the tree subscription which let the specifier define how the tree structure is organized. We won't define any magic algorithm to create a tree, it's the plain old tree representation which can be organized any way we want. It may not follow EMF's EObject#eContents topological structure

[florianbarbin]

The idea is to describe how we will represent selectable elements.
It will be similar to org.eclipse.sirius.components.collaborative.widget.reference.browser.ModelBrowsersDescriptionProvider which remove all non selectable leaf from the result in getChildren

[sbegaudeau]

I agree, and if it's not true, we will fallback on the existing selection subscription which means that this displayedAsTree boolean cannot be part of the selection subscription in the first place.

[florianbarbin]

See #3723 (comment)

[sbegaudeau]

This ADR has served its purpose as a support for the discussion, I'll merge it as is for now, it can be updated later to reflect the final state of the choices made.