Uploaded image for project: 'Magnolia UI'
  1. Magnolia UI
  2. MGNLUI-606

info.magnolia.ui.vaadin.integration.jcr whole package need review

XMLWordPrintable

    • Icon: Task Task
    • Resolution: Obsolete
    • Icon: Neutral Neutral
    • None
    • 5.0
    • framework

      It's an abstract mess in general grown over time. This could be simplified a lot by simple refactorings, documentation and probably get rid of some classes.

      all those methods in there take a node or an Id to perform actions, but the the classes are wrapping/adapting a single property or node, so you could theoretically create an adapter and pass a not relating node into it. Why pass those references around?


      info.magnolia.ui.vaadin.integration.jcr.JcrPropertyAdapter
      is using the same lists as the node adapters to keep track of changed properties. The properties beeing used inside a property are:

      • ModelConstants.JCR_NAME
      • JcrPropertyAdapter#VALUE_PROPERTY
      • JcrPropertyAdapter#TYPE_PROPERTY
        It's very confusing. There are properties inside properties. They override the same abstract functions, but do different things.
        The constants are locally stored, but also using ModelConstants.

      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter#changedProperties
      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter#removedProperties


      methods doing almost the same but not really, confusing:
      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#updateProperty
      info.magnolia.ui.vaadin.integration.jcr.JcrPropertyAdapter#updateProperty

      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#getItemProperty
      info.magnolia.ui.vaadin.integration.jcr.JcrPropertyAdapter#updateProperty


      property and nodes using getJcrItem, which is not a good idea from a performance perpective. And we do know on which type of item we are working on, so change it to getProperty and getNode. We end up casting every time we get a property or node from the repo.
      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter#getJcrItem


      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#getNode
      contains these lines of code:
      // get Node from repository
      node = (Node) getJcrItem();
      50 lines earlier, we have a method:
      info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#getNodeFromRepository() {
      return (Node) getJcrItem();


      DefaultProperty is used for creating an actual default property based on some default value but mostly it's used to create properties with non-default values. The term default may be misleading. same goes for DefaultPropertyUtil.
      As we're using generics in DefaultProperty now, maybe introducing generics in Adapters would make sense?

      DefaultPropertyUtil vs info.magnolia.jcr.util.PropertyUtil


      From code MGNLUI-568 review:
      The putting/removal of a property from the changedProperties and removedProperties HashMap in info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter should be synchronized or a synchronized map could be used instead.

      A marginal remark
      the while loop at info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter.updateProperties(Item) could be rewritten as a for loop

      for (Entry<String, Property> entry : changedProperties.entrySet()) {
      ...
      }
      

      ATM, removing properties will clear removedProperties HashMap, however, applying changes to (saving) properties doesn't remove properties from the changedProperties HashMap.

        Acceptance criteria

              Unassigned Unassigned
              ejervidalo Espen Jervidalo
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Created:
                Updated:
                Resolved:

                  Task DoR