[MGNLUI-3326] Support sorting by column in trees Created: 21/Jan/15  Updated: 27/Sep/16  Resolved: 10/Sep/15

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: 5.3.6
Fix Version/s: 5.3.11, 5.4.2

Type: Improvement Priority: Neutral
Reporter: Richard Gange Assignee: Jaroslav Simak
Resolution: Fixed Votes: 1
Labels: requires_documentation, tree
Remaining Estimate: 0d
Time Spent: 1d 7.5h
Original Estimate: Not Specified

Issue Links:
causality
is causing MGNLUI-3577 Sortable property shouldn't sort by d... Closed
dependency
is depended upon by MGNLUI-3543 Enable sorting in security app Closed
duplicate
is duplicated by MGNLUI-106 Implement sorting in trees Closed
supersession
is superseded by MGNLUI-3574 Add a sorting popup so that all trees... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Story Points: 5

 Description   

In particular, the HierarchicalJcrContainer needs to support sorting, so that we can enable it selectively in some apps. This is done via a configuration flag in TreePresenterDefinition for the time being.

See MGNLUI-106 for more details and original concept.

Also mind that this is different from sorting in lists, which is always on, as per column definitions.



 Comments   
Comment by Edgar Vonk [ 25/Jan/15 ]

Hi, I implemented this improvement in our own extension of the HierarchicalUserManager as follows. I think the Magnolia HierarchicalUserManager could be improved similarly?

	@Override
	protected Node createUserNode(String path, String userName, Session session) throws RepositoryException {
		Node userNode = super.createUserNode(path, userName, session);

		// order 1st and 2nd level parent folders alphabetically
		// e.g. order '/e/ed' before '/e/ee'
		// and order '/e' before '/f'
		Node parentNode = userNode.getParent();
		if (null != parentNode) {
			reorderNodeAlphabetically(parentNode);
		}
		Node parentParentNode = parentNode.getParent();
		if (null != parentParentNode) {
			reorderNodeAlphabetically(parentParentNode);
		}
		// order user node itself alphabetically
		reorderNodeAlphabetically(userNode);

		return userNode;
	}

	/**
	 * Re-orders the specified node alphabetically by comparing its name with existing sibling nodes.
	 *
	 * It re-orders the specified node before the first sibling whose name alphabetically comes after
	 * the specified node name. E.g. a node with name 'aa' is ordered before an existing node with name 'ab' etc.
	 *
	 * So this method assumes that the existing sibling nodes are already sorted alphabetically.
	 *
	 * @param node the node to reorder
	 * @throws RepositoryException if the reordering fails
	 */
	public static void reorderNodeAlphabetically(Node node) throws RepositoryException {
		Iterable<Node> siblings = NodeUtil.getSiblings(node);
		for (Node sibling : siblings) {
			if (node.getName().compareToIgnoreCase(sibling.getName()) < 0) {
				LOG.debug("Ordering node '{}' before existing node: '{}'", node.getName(), sibling.getName());
				NodeUtil.orderBefore(node, sibling.getName());
				break;
			}
		}
	}
Comment by Andreas Weder [ 02/Apr/15 ]

I've been asked to comment on this issue (and the linked support issue).

We've actually always wanted to have sorting in trees. The idea was that you'd just click in the column header, as you would do in the List view, to sort e.g. node names alphabetically in ascending, then descending order. To still allow for customer ordering, a third click in the "node name" column would switch back to the manual order and hide the arrow in the header, indicating the sort direction.

I would favor such a solution over defining an implicit sort order, but I've learned back then that implementing this could be tricky, partially at least due to deficiencies in Vaadin's TreeTable component, if I remember correctly. Since that time, we've basically been waiting for the (hierarchical) Grid component in Vaadin to appear. Grid is now here since 7.4, but it doesn't yet support hierarchical data sets.

I would not want to generally sort nodes in trees alphabetically (that breaks use cases e.g. in Pages), but if we'd restrict that to the Security app, that might work. I must admit I don't know all use cases, though, to tell for sure.

Comment by Edgar Vonk [ 02/Apr/15 ]

Hi @andreas. Thanks for the reply! We are in the progress of implementing a custom Content App for which we also want to have sorting in the tree view (without this the app is quite unusable at the moment) so if you have any idea on how we could best accomplish this that would be great.

For now I think we will go for a similar route as the one I gave in this issue: actually sort the nodes themselves when adding new nodes.

Comment by Roman Kovařík [ 03/Aug/15 ]

Review:

  • OrderedJcrContainer
    • ItemNameComparator is copy of info.magnolia.ui.workbench.tree.HierarchicalJcrContainer.ItemNameComparator. Please reuse the existing one.
    • Same for isJcrOrMgnlProperty.
    • getChildren missing override annotation
    • formatting
  • OrderedJcrContainerTest
    • Fix javadoc.
    • Switched assertions: assertEquals(expected, actual)
  • SecurityModuleVersionHandler
    • Typo in the task name.
    • You can use OrderedTreePresenterDefinition.class.getName() to prevent a typo (and you can do that also in the tests)
    • Shouldn't a nodeType be set to the definition?
  • The implementation could be useful not only in the security app and should probably go to magnolia-ui-workbench.
Comment by Evzen Fochr [ 03/Aug/15 ]
  • OrderedJcrContainer
    • changed in parent class to protected and removed
    • same
    • it isn't clean overwrite there is change in method how are children get (from getNodes() method to sql query)
    • done
  • OrderedJcrContainerTest
    • fixed
    • switched
  • SecurityModuleVersionHandler
    • fixed
    • done
    • normaly it is set by OrderedTreePresenterDefinition in OrderedTreePresenter
  • moved
Comment by Roman Kovařík [ 04/Aug/15 ]

Review: Seems OK, the final review should be probably done by jsimak or someone from the former platform cell.

Comment by Mikaël Geljić [ 05/Aug/15 ]

As far as I know, sorting isn't that much of an issue in the Vaadin world; doing so with our JCR containers is.
That's why unfortunately those presenters/containers have been extended way too much in several modules/apps (and everytime I spot a new one, a baby panda cries).

If sorting were implemented at least for the HierarchicalJcrContainer for now, then JcrContentConnectorDefinition's defaultOrder property would also become more useful...
Has this been researched by any chance? — If it is too cumbersome, I can't blame anyone for extending, rather blame the framework.
Mind that apparently, Edgar was kind enough to close the support request, so we might have a little bit more time to do this properly (and not only benefit the security app).

What do you guys think?

Comment by Andreas Weder [ 21/Aug/15 ]

Linking a page describing the actual interaction design for how we plan to implement this. Let's discuss the non-technical issues there.

Comment by Andreas Weder [ 21/Aug/15 ]

mgeljic had Finished extracting the design to the wiki page. I've decided to also remove it from this issue to avoid confusion and waisted efforts.

I've also promoted the manual ordering of notes to simply become one of the available sort criteria - see the updated mockups on the page.

edgar You might also want to have a look at our solution and provide feedback. Thanks for your input and solution.

Comment by Andreas Weder [ 09/Sep/15 ]

Added a more detailed description to capture the current state and in order to close the other tree sorting issue.

Generated at Mon Feb 12 09:05:30 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.