Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tui: mutations #3557

Merged
merged 5 commits into from
Apr 9, 2020
Merged

tui: mutations #3557

merged 5 commits into from
Apr 9, 2020

Conversation

oliver-sanders
Copy link
Member

These changes partially address #3529

  • Implement basic hard-coded mutations in Cylc Tui.
  • Obtain cycle points using cyclePoints: familyProxies(ids: ["root"]) trick.
  • Bonus testing.

This should open up the path to getting Cylc out for alpha testing.

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).
  • No change log entry required (there is already a changelog for Tui).
  • No documentation update required.

@oliver-sanders oliver-sanders added this to the cylc-8.0a2 milestone Apr 7, 2020
@oliver-sanders oliver-sanders self-assigned this Apr 7, 2020
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice. Minor comments, no blockers.

@oliver-sanders I accidentally hit Enter twice and held my workflow. Would a default empty state (e.g. "Select one of the options below" or similar) be useful to prevent users accidentally triggering mutations?

Also, I decided to try clicking Enter a lot of times - more than 10, less than 20 times. And managed to lock the cylc tui screen again, having to kill it. That's the same issue we had before, so not a blocker for this, just in case that's important.

image

Another thing too, is that if you stop the suite, and then click enter on cylc tui screen you get several Action panels.

image

Approved 🎉

cylc/flow/tui/util.py Show resolved Hide resolved
cylc/flow/tui/util.py Outdated Show resolved Hide resolved
{'user': ['a'], 'workflow': ['b'], 'cycle_point': ['c'],
'task': ['d'], 'job': ['e']}

"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions look useful (I feel like I had to write something similar in Cylc UI). Maybe later this will be promoted out of the tui module and be used by other code in Cylc Flow (or even Cylc UI Server which imports Cylc Flow).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is crossover between the two UIs, sadly working with different languages it's difficult to port one to the other.

TUI should always be simple and lightweight, I'd expect the WUI to vastly outstrip it in complexity.

@kinow
Copy link
Member

kinow commented Apr 7, 2020

(Some functional test appears to be failing, but this change doesn't seem to change anything in the code of Cylc Flow, only in the tui package, so ignoring that... also cannot kick Travis, 🤷‍♂️ )

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Apr 8, 2020

Added a default "cancel" button to the actions overlay to avoid accidental mutation.

I couldn't replicate the multiple action boxes or freeze bugs. Let me know if they occur again.

@wxtim wxtim self-requested a review April 8, 2020 13:54
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def review():
  if you_wanted_thorough_code_review:
      request_detailed_explanation()
      return False
  else:
      return {
        'Functionally tested': True,
        'Bruno\'s comments addressed': True
      }

It worked with my deliberately horrible (800 tasks per cycle) workflow.

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well for me! Nice work ✔️

@dwsutherland dwsutherland merged commit e904325 into cylc:master Apr 9, 2020
@oliver-sanders oliver-sanders deleted the tui-mutation branch May 27, 2020 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants