Attempt 2 collapsing gscan

[AaronDCole]

Attempt number 3 at the auto collapse workflows, this may not work flawlessly yet but would be good to get some review on based on the feedback from Oliver on the previous pull request (#1037).

This is a ground up rewrite of the original feature request, based on the latest changes on developer (including the most recent changes from Oliver), with (hopefully) a lot of code quality improvements and general readability updates (since I found the structure of the g-scan element incredible hard to follow / change in a recursive fashion (as suggested in Olivers comments on the previous MR, listed above).

Tests will be broken, but an initial review would be helpful (even if just a sighting pass), then Ill make final adjustments and then fix the tests.

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).

• Appropriate tests are included (unit and/or functional).
• Already covered by existing tests.
• Does not need tests (why?).

• Appropriate change log entry included.
• No change log entry required (why? e.g. invisible to users).

• I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.
• No documentation update required.

[oliver-sanders]

Thanks @AaronDCole.

I'm not terribly keen on the HTML line break removals which are also making it hard to work out exactly what this PR has changed. It would be easier to consider code style changes in a separate issue/PR (see also #1172), we may also consider using prettier (or similar) with Vue support for HTML/SCSS formatting.

The auto-collapsing works well for the examples 👍, although for some strange reason, when I tried it with a workflow named generic/run1 it didn't work and the workflow icon broke?

I also spotted an issue with the workflow names when these nodes are expanded (more in later comment):

The "Tree Component" src/components/cylc/tree/Tree is used by both the "GScan Component" and the "Tree View" (src/views/Tree) which has the unfortunate side effect that this auto-collapsing also impacts the tree view which is undesirable. For now we can use a simple toggle switch to turn this off (more in later comment).

[oliver-sanders]

Please don't collapse code down to one long line like this, it's bad for reading/editing and makes it hard to understand the diff. I don't think anything here has actually changed?

[AaronDCole]

Sorry you're totally right, I didn't actually intend to include that and I will fix/revert that in the next commit.

[oliver-sanders]

This will be invoked every time anything in node (or any of its children) changes which will be very compute intensive.

I think this can be handled with a computed property:

// untested
computed: {
  autoExpand () {
    // only called when one of its dependencies changes
    if (!this.firstInteraction) {
      return null
    }
    return this.hasBranchingLineage && this.autoExpandTypes.includes(this.node.type)
  }
},
watch: {
  autoExpand (expand) {
    // only called when autoExpand changes
    if (expand !== null) {
      this.emitExpandCollapseEvent(expand)
    }
  }
}

Note that we don't (initially) want this behaviour in the "Tree view" (src/Tree) which also shares this recursive tree component. Although one day we may want to auto-collapse tasks with only one job in a similar manner.

For now it's probably good enough to target this behaviour at the workflow-part type, the recursive tree component needs straightening out so we can refactor this branch away later. E.G:

// untested
computed: {
  autoExpand () {
    // only called when one of its dependencies changes
    if (!this.firstInteraction) {
      return null
    }
    if (node.type === 'workflow-part') {
      return this.hasBranchingLineage && this.autoExpandTypes.includes(this.node.type)
    }
    return this.autoExpandTypes.includes(this.node.type)
  }
},

[oliver-sanders]

checkForBranchingLineage and getLastDescendent seem to be used multiple times which means there will be duplicate computation. It would be worth using computed properties to cache these values to avoid unnecessary computation.

[AaronDCole]

'The auto-collapsing works well for the examples 👍, although for some strange reason, when I tried it with a workflow named generic/run1 it didn't work and the workflow icon broke?'

I actually worked out why this was happening after I left the office last night, I will fix it today

[oliver-sanders]

I think this test might need to use this.isExpanded so that the name collapsing is only performed when the node is collapsed.

<span>{{ (this.isExpanded && checkForBranchingLineage(scope.node)) ? scope.node.name || scope.node.id : createDescendentLabel(scope.node) }}</span>

E.G. the collapsed state should look like this:

b/b1/b11

The expanded state should look as it did before:

b/
  b1/
    b11/

Rather than:

b/b1/b1
  b1/b11
    b11/

[AaronDCole]

Good point!

[oliver-sanders]

This feature can be tested in a component test but at the moment the code structure doesn't allow this (because we would have to mock all the GraphQL/VueX data stuff). I've split the GraphQL/VueX stuff out of the GScan component into a GScan view to allow the component to be tested standalone.

Feel free to cherry-pick this commit, it contains a blank component test with the a/a1 example workflow pre-loaded.

https://github.com/cylc/cylc-ui/compare/master...oliver-sanders:cylc-ui:gscan-component?expand=1

There are a couple of simple component tests to look at but we haven't started using them widely yet. You should be able to cy.get the expand/collapse button and cy.click it, you can tell if a row is collapsed using cy.should('be.visible') or by inspecting the vm and looking at the isExpanded attribute.

[AaronDCole]

This feature can be tested in a component test but at the moment the code structure doesn't allow this (because we would have to mock all the GraphQL/VueX data stuff). I've split the GraphQL/VueX stuff out of the GScan component into a GScan view to allow the component to be tested standalone.

Feel free to cherry-pick this commit, it contains a blank component test with the a/a1 example workflow pre-loaded.

https://github.com/cylc/cylc-ui/compare/master...oliver-sanders:cylc-ui:gscan-component?expand=1

There are a couple of simple component tests to look at but we haven't started using them widely yet. You should be able to cy.get the expand/collapse button and cy.click it, you can tell if a row is collapsed using cy.should('be.visible') or by inspecting the vm and looking at the isExpanded attribute.

Thanks Oliver, I am refactoring a lot of the commit (to, for example, be able to pass through computed properties from the TreeItem component to use where the slot overrides are defined in the gScan component), so I will look into this when that is finished.

[AaronDCole]

There is an issue where when you switch to the tree view, it seems to change the format of the node object which breaks a lot of stuff within the gscan component. I will investigate tomorrow.

[hjoliver]

Taking a look today ...

[hjoliver]

@AaronDCole - the placement of job icons looks good now as I collapse and expand the tree, but I've noticed a couple of problems.

I tested it side-by side with master, by moving the dist directory from building this branch to dist-scan, and on master to dist-master, then starting two UIs:

$ cylc gui --CylcUIServer.ui_build_dir="/home/oliverh/cylc/cylc-ui/dist-gscan"
$ cylc gui --new --CylcUIServer.ui_build_dir="/home/oliverh/cylc/cylc-ui/dist-master"

Then I started 4 copies of this workflow:

[scheduling]
   cycling mode = integer
   runahead limit = P2
   [[graph]]
      P1 = "a => b1 & b2 & b3 "
[runtime]
   [[a, b1, b2, b3]]
       script = sleep $((10 + RANDOM % 5))

... at different levels of run-dir nesting, like this:

$ cylc vip  # bgg/run1
$ cylc vip  # bgg/run2
$ cylc vip --workflow-name=top/mid/one  # top/mid/one/run1
$ cylc vip --workflow-name=top/mid/two  # top/mid/two/run1

The two problems are:

• the workflow icon shouldn't appear at the 2nd level from the bottom (see red arrow, below)
• the job icon state totals aren't responding properly at the lowest level (see red loop, below). In my test cases here, for instances, the tooltip always says 5 running, even if there are none running (watch out for this bug if you pause a workflow, but the problem is more than that: datastore wrong when workflow paused? cylc-flow#5417

To get the state totals right you could put some really long sleep values in some tasks, so that the totals stay stable for a long time. And use cylc scan -t rich --colour-blind or cylc dump -t [workflow-ID] to see the right numbers per workflow.

(And of course, at the higher levels - as you collapse tree nodes - the state totals should be a sum of the individual workflows below).

[hjoliver]

Tests pass 🎉 I'm going to build and test later this afternoon...

[hjoliver]

This is looking good. I'll try to find time to leave some review comments this afternoon, and to prompt others on the team to review it. (Bearing in mind the Vue3 changes in master now).

[hjoliver]

superseded by #1406