Major problems preventing Content Apps from being JCR-agnostic
-
MGNLUI-2662
-
Getting issue details...
STATUS
Action Executor API. We don't debate whether
ActionExecutor
should or should not be responsible for primary action availability checking , but it shouldn't do it _exclusively_ forjavax.jcr.Items
.- Should work over Vaadin Items. Being used in both Browser and Detail sub-apps of Content App it is probably the most severe blocker for the task.
- Availability rules are all affected, but easy to fix, because those that depend on JCR can simply cast to
JcrItemAdapter
/JcrNodeAdapter
. - Still not quite clear what to do with JCR logic inside AbstractActionExecutor itself.
- Serialization/de-serialization of item ids in to strings (basically - URL fragments).
The snippets like the one below are not only JCR-dependent, but also cluttered and hard to understand. What is more - the similar pieces of code are common on all the levels of Content App presenters, sometimes maybe even not once.
URL Fragment to Item Id Transformation in Browser Sub-AppString itemId = null; try { itemId = JcrItemUtil.getItemId(SessionUtil.getNode(workspaceName, path)); // item might have not been found if path doesn't exist if (itemId == null) { itemId = JcrItemUtil.getItemId(SessionUtil.getNode(workspaceName, workbenchRoot)); BrowserLocation newLocation = getCurrentLocation(); newLocation.updateNodePath("/"); getAppContext().updateSubAppLocation(getSubAppContext(), newLocation); } } catch (RepositoryException e) { log.warn("Could not retrieve item at path {} in workspace {}", path, workspaceName); }
Instead we could simply always delegate fragment to id translation (and vice versa) to the component which is specialised on that.
How the same logic should be probably doneObject actualItemId = dataSourceManager.deserializeItemId(urlFragmentPath); // item might have not been found if path doesn't exist if (dataSourceManager.itemExists(actualItemId)) { actualItemId = dataSourceManager.getRootItemId(); BrowserLocation newLocation = getCurrentLocation(); newLocation.updateNodePath("/"); getAppContext().updateSubAppLocation(getSubAppContext(), newLocation); } return actualItemId;
- Mixed signatures of methods in Content App related presenters and events
- Some operate over Vaadin Items, some - over the
JcrItemAdapters
, some, like aforementioned, - over JCR Items themselves. It sometimes leads to ItemId <-> Item conversion several times during delegation.- As a solution - only Item Ids should be used as a transferable during components/apps/sub-apps communication.
- Only workbench knows how to transform Item Id into a Vaadin Item. Ideally - that should be a job of a separate datasource manager component as such functionality is need in various places (e.g. Detail Sub-apps).
- Strings as Item ids everywhere - Object should be used instead (generics would be even better, but that's rather hard to implement with limited support of them in Vaadin Data binding, Dependency Injection etc).
- Some operate over Vaadin Items, some - over the
- JCR-specific terms popping up constantly (workspace, workbench root etc) - can be generalised and/or encapsulated.
- Things that should also be moved out from
BrowserSubApp
in order to make it purely JCR-agnostic (without even child JCR-specific classes)- Application of inline-editing (
BrowserPresenter#editItem
). - Action availability verification (e.g.
BrowserSubAppBase#isSectionVisible
).
- Application of inline-editing (
- Provided the above is resolved -
DetailSubApp
comes JCR-agnostic pretty much for free.- That holds also for most of Dialogs (except for chooser probably, but that's to be researched).
- Field factories are still slightly bound to JCR due to the
BasicTransformer
, but that not the biggest problem - Transformer concept still can be used for cases of other data sources and to some extent custom solution would be needed.
JCR-Agnostic content app prototype patches
0001-jcr-agnostic-content-app.patch
Usage Instructions:
- All the affected tests are adapted at least compilation wise, but I suggest -DskipTests flag is used.
- Some checkstyle leftovers might occur, so -Dcheckstyle.ignore=true is also a good idea.
- I suggest using magnolia-empty web-app for testing the concept out as some of the modules (e.g. sitemaps, rss-agg) might need re-build to work correctly.
Overview
Content Tools