[MGNLUI-606] info.magnolia.ui.vaadin.integration.jcr whole package need review Created: 24/Jan/13  Updated: 10/Mar/21  Resolved: 10/Mar/21

Status: Closed
Project: Magnolia UI
Component/s: framework
Affects Version/s: 5.0
Fix Version/s: None

Type: Task Priority: Neutral
Reporter: Espen Jervidalo Assignee: Unassigned
Resolution: Obsolete Votes: 0
Labels: adapters
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-1790 Item of JcrNewNodeAdapter should poin... Closed
Template:
Acceptance criteria:
Empty
Task DoR:
Empty
Date of First Response:

 Description   

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.



 Comments   
Comment by Daniel Lipp [ 05/Feb/13 ]

re "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."

I actually think it's safe to not synchronize there. Those adapters are pretty transient and basically created on the fly so IMHO the only situation where an instance is used twice is, when editing an item of a table in a Dialog. As dialogs are modal to the table this should still be safe. I cannot think of any situation where different users would end up with the same instance of such an adapter.

Generated at Mon Feb 12 08:38:29 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.