-
Task
-
Resolution: Obsolete
-
Neutral
-
None
-
5.0
-
-
Empty show more show less
-
Empty show more show less
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.
- relates to
-
MGNLUI-1790 Item of JcrNewNodeAdapter should point the newly created node after applying changes (not to parent node)
- Closed
- mentioned in
-
Wiki Page Loading...